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: sage-8.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:

Status badges

Description (last modified by vdelecroix)

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 vdelecroix

  • Authors set to Vincent Delecroix
  • Branch set to u/vdelecroix/25512
  • Commit set to b2fa88c45770fbae735069e04e57182d01948d4a
  • Status changed from new to needs_review

New commits:

4ab32e224523: "Real Field" -> "Real Floating-point Field"
b2fa88c25512: is_square for Laurent polynomials

comment:2 Changed 3 years ago by git

  • Commit changed from b2fa88c45770fbae735069e04e57182d01948d4a to 193df329ede18fad67212e211c6ee05ff91fc3da

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

193df3225512: is_square for Laurent polynomials

comment:3 Changed 3 years ago by git

  • Commit changed from 193df329ede18fad67212e211c6ee05ff91fc3da to 2ff881856b6266f90a1b917fb037e8eece1097b2

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2ff881825512: is_square for Laurent polynomials

comment:4 follow-up: Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

While this

return (True, LaurentPolynomial_univariate(self._parent, r, self.__n // 2))

is okay for now, it is not future-proof 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 ; follow-up: Changed 3 years ago by vdelecroix

Replying to tscrim:

While this

return (True, LaurentPolynomial_univariate(self._parent, r, self.__n // 2))

is okay for now, it is not future-proof 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 ; follow-up: Changed 3 years ago by 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 future-proof 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 git

  • Commit changed from 2ff881856b6266f90a1b917fb037e8eece1097b2 to 5e1cadf1b2c433aa810bf11093884f647859ae27

Branch pushed to git repo; I updated commit sha1. New commits:

5e1cadf25512: _new_c method + various cleaning

comment:8 in reply to: ↑ 6 Changed 3 years ago by vdelecroix

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 future-proof 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 vdelecroix

  • Description modified (diff)

comment:10 Changed 3 years ago by vdelecroix

  • 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 vdelecroix

  • Dependencies set to #25524
  • Description modified (diff)

Cleaning (#25524) is set as a dependency.

comment:12 Changed 3 years ago by git

  • Commit changed from 5e1cadf1b2c433aa810bf11093884f647859ae27 to 093d8a3be8559eb9a1bc6b2018840e8c8a41d2ac

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

028a39e25524: cleaning Laurent polynomials
20291d825524: some fixes to sage/algebras
9cacb5a25524: fix doctests in algebras
093d8a325512: is_square for Laurent polynomial

comment:13 Changed 3 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:14 Changed 3 years ago by git

  • Commit changed from 093d8a3be8559eb9a1bc6b2018840e8c8a41d2ac to 7e3870da438fcc1074eb8362ed518a509db697c5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2c4184725524: cleaning Laurent polynomials
19ca66625524: some fixes to sage/algebras
a9b43eb25524: fix doctests in algebras
228529a25524: add linebreak to doctests
7e3870d25512: is_square for Laurent polynomial

comment:15 Changed 3 years ago by git

  • Commit changed from 7e3870da438fcc1074eb8362ed518a509db697c5 to ea27ddeb5d4809b13c44f9c1325ba41bb21b50db

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ea27dde25512: is_square for univariate Laurent polynomial

comment:16 Changed 3 years ago by vdelecroix

  • Dependencies #25524 deleted
  • Milestone changed from sage-8.3 to sage-8.4

comment:17 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

LGTM. Thank you.

comment:18 Changed 3 years ago by vbraun

  • Branch changed from u/vdelecroix/25512 to ea27ddeb5d4809b13c44f9c1325ba41bb21b50db
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.