Opened 3 years ago
Closed 3 years ago
#25512 closed enhancement (fixed)
Add is_square method to LaurentPolynomial_univariate class
Reported by:  vklein  Owned by:  

Priority:  major  Milestone:  sage8.4 
Component:  algebra  Keywords:  
Cc:  vdelecroix  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  ea27dde (Commits, GitHub, GitLab)  Commit:  ea27ddeb5d4809b13c44f9c1325ba41bb21b50db 
Dependencies:  Stopgaps: 
Description (last modified by )
Add an is_square instance method to LaurentPolynomial_univariate
class.
This ticket is a prerequisite for #24677.
Change History (18)
comment:1 Changed 3 years ago by
 Branch set to u/vdelecroix/25512
 Commit set to b2fa88c45770fbae735069e04e57182d01948d4a
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 Commit changed from b2fa88c45770fbae735069e04e57182d01948d4a to 193df329ede18fad67212e211c6ee05ff91fc3da
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
193df32  25512: is_square for Laurent polynomials

comment:3 Changed 3 years ago by
 Commit changed from 193df329ede18fad67212e211c6ee05ff91fc3da to 2ff881856b6266f90a1b917fb037e8eece1097b2
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2ff8818  25512: is_square for Laurent polynomials

comment:4 followup: ↓ 5 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
While this
return (True, LaurentPolynomial_univariate(self._parent, r, self.__n // 2))
is okay for now, it is not futureproof if we have a subclass of LaurentPolynomial_univariate
. It would be better to do
return (True, type(self)(self._parent, r, self.__n // 2))
Bikeshedding (so feel free to ignore), but I'd rather see elif root:
> if root:
to better indicate that the previous block is meant to stop.
comment:5 in reply to: ↑ 4 ; followup: ↓ 6 Changed 3 years ago by
Replying to tscrim:
While this
return (True, LaurentPolynomial_univariate(self._parent, r, self.__n // 2))is okay for now, it is not futureproof if we have a subclass of
LaurentPolynomial_univariate
.
On the other hand, all other methods create new Laurent polynomial this way. I don't have any strong opinion on the way it should be done but I have a strong opinion on the fact that it should be the same in all methods. If you feel like it has to change, it would better be part of another ticket as I don't want to modify other methods.
It would be better to do
return (True, type(self)(self._parent, r, self.__n // 2))Bikeshedding (so feel free to ignore), but I'd rather see
elif root:
>if root:
to better indicate that the previous block is meant to stop.
For this I don't agree. It is much easier to read a if/elif/elif/else
rather a if/if/if/if
where you have to think whether the other blocks meant to stopped.
comment:6 in reply to: ↑ 5 ; followup: ↓ 8 Changed 3 years ago by
Replying to vdelecroix:
Replying to tscrim:
While this
return (True, LaurentPolynomial_univariate(self._parent, r, self.__n // 2))is okay for now, it is not futureproof if we have a subclass of
LaurentPolynomial_univariate
.On the other hand, all other methods create new Laurent polynomial this way. I don't have any strong opinion on the way it should be done but I have a strong opinion on the fact that it should be the same in all methods. If you feel like it has to change, it would better be part of another ticket as I don't want to modify other methods.
I have a strong opinion that it should be done this way and to not to make things worse on new code. Doing things a bad way just because it was done that way in the past is not a good argument.
comment:7 Changed 3 years ago by
 Commit changed from 2ff881856b6266f90a1b917fb037e8eece1097b2 to 5e1cadf1b2c433aa810bf11093884f647859ae27
Branch pushed to git repo; I updated commit sha1. New commits:
5e1cadf  25512: _new_c method + various cleaning

comment:8 in reply to: ↑ 6 Changed 3 years ago by
Replying to tscrim:
Replying to vdelecroix:
Replying to tscrim:
While this
return (True, LaurentPolynomial_univariate(self._parent, r, self.__n // 2))is okay for now, it is not futureproof if we have a subclass of
LaurentPolynomial_univariate
.On the other hand, all other methods create new Laurent polynomial this way. I don't have any strong opinion on the way it should be done but I have a strong opinion on the fact that it should be the same in all methods. If you feel like it has to change, it would better be part of another ticket as I don't want to modify other methods.
I have a strong opinion that it should be done this way and to not to make things worse on new code. Doing things a bad way just because it was done that way in the past is not a good argument.
Now it is clean :)
comment:9 Changed 3 years ago by
 Description modified (diff)
comment:10 Changed 3 years ago by
 Status changed from needs_review to needs_work
But apparently something is not working anymore... (or Iwahori Hecke algebra did use some forbidden features)
comment:11 Changed 3 years ago by
 Dependencies set to #25524
 Description modified (diff)
Cleaning (#25524) is set as a dependency.
comment:12 Changed 3 years ago by
 Commit changed from 5e1cadf1b2c433aa810bf11093884f647859ae27 to 093d8a3be8559eb9a1bc6b2018840e8c8a41d2ac
comment:13 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:14 Changed 3 years ago by
 Commit changed from 093d8a3be8559eb9a1bc6b2018840e8c8a41d2ac to 7e3870da438fcc1074eb8362ed518a509db697c5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2c41847  25524: cleaning Laurent polynomials

19ca666  25524: some fixes to sage/algebras

a9b43eb  25524: fix doctests in algebras

228529a  25524: add linebreak to doctests

7e3870d  25512: is_square for Laurent polynomial

comment:15 Changed 3 years ago by
 Commit changed from 7e3870da438fcc1074eb8362ed518a509db697c5 to ea27ddeb5d4809b13c44f9c1325ba41bb21b50db
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ea27dde  25512: is_square for univariate Laurent polynomial

comment:16 Changed 3 years ago by
 Dependencies #25524 deleted
 Milestone changed from sage8.3 to sage8.4
comment:17 Changed 3 years ago by
 Status changed from needs_review to positive_review
LGTM. Thank you.
comment:18 Changed 3 years ago by
 Branch changed from u/vdelecroix/25512 to ea27ddeb5d4809b13c44f9c1325ba41bb21b50db
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
24523: "Real Field" > "Real Floatingpoint Field"
25512: is_square for Laurent polynomials