Opened 4 years ago

Closed 4 years ago

#23932 closed defect (fixed)

differences in construction of PolynomialRing and LaurentPolynomialRing

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.1
Component: algebra Keywords:
Cc: Merged in:
Authors: Vincent Delecroix Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: d1d4431 (Commits, GitHub, GitLab) Commit: d1d443153ad6229577b2ce2cc61ba3986ff6250e
Dependencies: Stopgaps:

Status badges

Description (last modified by vdelecroix)

sage: LaurentPolynomialRing(QQ, 'w', 1)
Univariate Laurent Polynomial Ring in w over Rational Field
sage: PolynomialRing(QQ, 'w', 1)
Multivariate Polynomial Ring in w over Rational Field

The difference actually comes from a wrong __repr__ as

sage: type(LaurentPolynomialRing(QQ, 't')).__name__
'LaurentPolynomialRing_univariate_with_category'
sage: type(LaurentPolynomialRing(QQ, 't', 1)).__name__
'LaurentPolynomialRing_mpair_with_category'

While we are at it we clean the constructor. The attached branch calls directly PolynomialRing and then, depending whether the result is univariate or not, construct the relevant Laurent polynomial ring.

Change History (8)

comment:1 Changed 4 years ago by vdelecroix

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

New commits:

d1d443123932: clean LaurentPolynomialRing constructor

comment:2 Changed 4 years ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 4 years ago by tscrim

You should issue a warning saying this behavior has changed since, IIRC, the univariate polynomials have more functionality than multivariate.

I wonder what the prepend_string was originally used for...the comment about it is not so enlightening.

Last edited 4 years ago by tscrim (previous) (diff)

comment:4 Changed 4 years ago by vdelecroix

I should not. The previous version was wrongly writing univariate when it was multivariate with one variable...

-        if self._n == 1:
-            return "Univariate Laurent Polynomial Ring in %s over %s"%(self._R.variable_name(), self._R.base_ring())
-        else:
-            return "Multivariate Laurent Polynomial Ring in %s over %s"%(", ".join(self._R.variable_names()), self._R.base_ring())

comment:5 Changed 4 years ago by vdelecroix

  • Description modified (diff)

I updated the ticket description.

comment:6 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Yes, I agree. I should have checked the type before saying anything too.

Changes LGTM. This seems like a much needed cleanup.

comment:7 Changed 4 years ago by vdelecroix

That's also my fault that the description was unclear (I realized it while I was cleaning).

Thank for the review!

comment:8 Changed 4 years ago by vbraun

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