#23338 closed enhancement (fixed)

Clean up polynomial constructor

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.0
Component: algebra Keywords:
Cc: tscrim Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: fc75734 (Commits) Commit: fc75734d30afcbe5604ff835ae2d1bb5b983e713
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

This ticket cleans up the PolynomialRing() function in many ways. Mainly:

  1. Use normalize_names() as much as possible to deal with variable names.
  1. Fix indentation in docstring.
  1. Pass arguments like sparse and implementation as **kwds to the single-variate or multi-variate polynomial constructor.
  1. Make the code easier to understand.
  1. Allow PolynomialRing(..., implementation="singular") which forces Singular. This allows simplifying some code in src/sage/algebras/free_algebra.py which currently needs to jump through hoops to get a MPolynomialRing_libsingular.
  1. Check more error conditions, for example currently we have
    sage: PolynomialRing(QQ, name="x", names="y")
    Univariate Polynomial Ring in y over Rational Field
    
  1. Change some arguments of PolynomialRing() to keyword-only arguments. This does break some existing uses of PolynomialRing(), although much more likely in library code and not user code (some code in Sage gets this even wrong and was passing sparse="lex" instead of order="lex")

Apart from items 6 and 7, no existing functionality is changed.

Change History (28)

comment:1 Changed 23 months ago by tscrim

  • Cc tscrim added

+1

comment:2 Changed 23 months ago by jdemeyer

  • Branch set to u/jdemeyer/clean_up_polynomial_constructor

comment:3 Changed 23 months ago by jdemeyer

  • Commit set to 883d50709298487518d7d0a2771045483adb0cf9

First version, not finished or tested yet. But feel free to comment...


New commits:

883d507Clean up PolynomialRing() constructor

comment:4 Changed 23 months ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 23 months ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 23 months ago by jdemeyer

  • Description modified (diff)

comment:7 Changed 23 months ago by jdemeyer

  • Description modified (diff)

comment:8 Changed 23 months ago by jdemeyer

  • Description modified (diff)

comment:9 Changed 23 months ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 23 months ago by git

  • Commit changed from 883d50709298487518d7d0a2771045483adb0cf9 to 2f8a591a6b7151c442c9d20b53a5131eaa26e3b0

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

2f8a591Clean up PolynomialRing() constructor

comment:11 Changed 23 months ago by git

  • Commit changed from 2f8a591a6b7151c442c9d20b53a5131eaa26e3b0 to f0a314c800e0f060008a9402d1a7f2fba968920b

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

f0a314cClean up PolynomialRing() constructor

comment:12 Changed 23 months ago by jdemeyer

  • Description modified (diff)

comment:13 Changed 23 months ago by git

  • Commit changed from f0a314c800e0f060008a9402d1a7f2fba968920b to b410689bf2e5ef76ff9010f541a070d71778484f

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

b410689Clean up PolynomialRing() constructor

comment:14 follow-up: Changed 23 months ago by tscrim

My quick comments are:

  • The indentation of the doc looks off for the numbered points. The subsequent lines of text should start at the same place the first line did (unless you want it to be further indented).
  • In handling the case R.<x> = PolynomialRing(QQ, 'x'), you should normalize the names there so that kwnames != names is a valid comparison (e.g., names='x,y' should be valid input).
  • You also almost already check for R.<x> = PolynomialRing(QQ, 'x') in the no var_array block.
  • We probably can/should handle the R.<x> = PolynomialRing(QQ, 'x') input entirely in the no var_array block.
  • Should I interpret the [m, ...] as extra input here?
    4. ``PolynomialRing(base_ring, n, [m, ...,] var_array=var_array, ...)``
    
    It is confusing with what might be a list. It might be better to do:
    4. ``PolynomialRing(base_ring, n0, ..., nm, var_array=var_array, ...)``
    
  • Your code has some slightly sneaky logic for some corner cases like PolynomialRing(QQ, 'x', 1, var_array='x') not being valid input. It would be good to have some tests for var_array input.

comment:15 Changed 23 months ago by git

  • Commit changed from b410689bf2e5ef76ff9010f541a070d71778484f to 98d9d596260ccb224fcc565693b733b437dbab7a

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

98d9d59Clean up PolynomialRing() constructor

comment:16 in reply to: ↑ 14 ; follow-up: Changed 23 months ago by jdemeyer

Replying to tscrim:

  • In handling the case R.<x> = PolynomialRing(QQ, 'x'), you should normalize the names there so that kwnames != names is a valid comparison (e.g., names='x,y' should be valid input).

No, the current code is intentional. I only want to support

R.<x,y> = PolynomialRing(QQ, "x,y")

and not

R = PolynomialRing(QQ, "x,y", names="x,y")

The preparser passes names already normalized, so there is no reason to call normalize_names() on kwnames.

  • You also almost already check for R.<x> = PolynomialRing(QQ, 'x') in the no var_array block.

What do you mean with this?

  • We probably can/should handle the R.<x> = PolynomialRing(QQ, 'x') input entirely in the no var_array block.

Well, that would make the code slightly more complicated since that check needs to be done after normalize_names(). Also, I see no reason to a priori disallow something like

R.<x0,x1> = PolynomialRing(QQ, 2, var_array=["x"])

Yes, it's stupid to write that but we do allow the analogous thing without var_array.

  • Your code has some slightly sneaky logic for some corner cases like PolynomialRing(QQ, 'x', 1, var_array='x') not being valid input. It would be good to have some tests for var_array input.

Right. I didn't care much about var_array since it's not widely used (there isn't a single place in the Sage library or documentation where it is used, except for the PolynomialRing constructor itself). Still, you are right that additional doctests would be good.

comment:17 Changed 23 months ago by git

  • Commit changed from 98d9d596260ccb224fcc565693b733b437dbab7a to 9be4fc3ef59303385f7d5489e970441dfe81a212

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

9be4fc3Clean up PolynomialRing() constructor

comment:18 Changed 23 months ago by git

  • Commit changed from 9be4fc3ef59303385f7d5489e970441dfe81a212 to dfb59b3f87dac632ed040afceb0340800f8ceb6e

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

dfb59b3Clean up PolynomialRing() constructor

comment:19 Changed 23 months ago by jdemeyer

  • Status changed from new to needs_review

I think this addresses your comments, except for the ones regarding the redundant R.<x> = Polynomial(QQ, "x") syntax.

comment:20 in reply to: ↑ 16 ; follow-up: Changed 23 months ago by tscrim

Replying to jdemeyer:

Replying to tscrim:

  • In handling the case R.<x> = PolynomialRing(QQ, 'x'), you should normalize the names there so that kwnames != names is a valid comparison (e.g., names='x,y' should be valid input).

No, the current code is intentional. I only want to support

R.<x,y> = PolynomialRing(QQ, "x,y")

and not

R = PolynomialRing(QQ, "x,y", names="x,y")

The preparser passes names already normalized, so there is no reason to call normalize_names() on kwnames.

Ah, I see. Okay.

  • You also almost already check for R.<x> = PolynomialRing(QQ, 'x') in the no var_array block.

What do you mean with this?

Most of the code that checks for input of this form is already in the code path when var_array is not being used. I feel like it should go there. However, this is moot because...

  • We probably can/should handle the R.<x> = PolynomialRing(QQ, 'x') input entirely in the no var_array block.

Well, that would make the code slightly more complicated since that check needs to be done after normalize_names(). Also, I see no reason to a priori disallow something like

R.<x0,x1> = PolynomialRing(QQ, 2, var_array=["x"])

Yes, it's stupid to write that but we do allow the analogous thing without var_array.

I agree with this. I missed the normalize_names call on my quick look through.

  • Your code has some slightly sneaky logic for some corner cases like PolynomialRing(QQ, 'x', 1, var_array='x') not being valid input. It would be good to have some tests for var_array input.

Right. I didn't care much about var_array since it's not widely used (there isn't a single place in the Sage library or documentation where it is used, except for the PolynomialRing constructor itself). Still, you are right that additional doctests would be good.

I did the initial implementation because I got a few requests to have such a feature (which is useful for doing matrices with generic parameters). So I care a little bit. :)

I'm wondering if we should always have a one variable implementation always be a univariate polynomial ring, of course with implementation='singular' being an exception to construct the multivariate polynomial ring? This should be uncontroversial as it only expands functionality.

comment:21 in reply to: ↑ 20 Changed 23 months ago by jdemeyer

Replying to tscrim:

I'm wondering if we should always have a one variable implementation always be a univariate polynomial ring, of course with implementation='singular' being an exception to construct the multivariate polynomial ring? This should be uncontroversial as it only expands functionality.

I don't agree that it only expands functionality. In at least 2 places, the Sage library has been using PolynomialRing(R, 1, "x") as a contrived way to get a multivariate polynomial ring implemented in Singular. Of course, this can now be done using PolynomialRing(R, "x", implementation="singular"). For this reason, I would maybe give a deprecation warning for PolynomialRing(R, 1, "x") but not change functionality yet.

comment:22 Changed 23 months ago by jdemeyer

It seems that there are quite a bit of places in Sage which rely on the behaviour that specifying 1 variable gives a multivariate polynomial ring. For example, doctests checking whether some algorithm for multivariate rings also deals with the 1-variable case properly. So I feel inclined to keep the current behaviour.

comment:23 Changed 23 months ago by tscrim

Fair enough. Although in some ways changing the behavior makes the doctests a little moot...but one could directly call the constructor class (or pass implementation="singular"). However, it is beyond the scope of the ticket in a way, so I'm fine doing this on another ticket.

comment:24 follow-up: Changed 23 months ago by tscrim

So it looks like we took care of all places that are using PolynomialRing(F, 1, 'x') to get a Singular object other than some doctests in polynomial_singular_interface.py and interfaces/singular.py. Once those are changed, then you can set this to a positive review.

comment:25 in reply to: ↑ 24 Changed 23 months ago by jdemeyer

Replying to tscrim:

So it looks like we took care of all places that are using PolynomialRing(F, 1, 'x') to get a Singular object other than some doctests in polynomial_singular_interface.py and interfaces/singular.py.

I actually think that there are more places like that. I'll try to fix them all.

comment:26 Changed 23 months ago by git

  • Commit changed from dfb59b3f87dac632ed040afceb0340800f8ceb6e to fc75734d30afcbe5604ff835ae2d1bb5b983e713

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

fc75734Use implementation="singular" instead of using 1 variable to force a Singular polynomial ring

comment:27 Changed 23 months ago by tscrim

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

Thanks. LGTM.

comment:28 Changed 22 months ago by vbraun

  • Branch changed from u/jdemeyer/clean_up_polynomial_constructor to fc75734d30afcbe5604ff835ae2d1bb5b983e713
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.