Clean up polynomial constructor

[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.

2. Fix indentation in docstring.

3. Pass arguments like sparse and implementation as **kwds to the single-variate or multi-variate polynomial constructor.

4. Make the code easier to understand.

5. 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.

6. Check more error conditions, for example currently we have

sage: PolynomialRing(QQ, name="x", names="y")
Univariate Polynomial Ring in y over Rational Field

7. 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.

CC: @tscrim

Component: algebra

Author: Jeroen Demeyer

Branch/Commit: fc75734

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/23338

[tscrim]

comment:1

+1

[jdemeyer]

Branch: u/jdemeyer/clean_up_polynomial_constructor

[jdemeyer]

Commit: 883d507

[jdemeyer]

comment:3

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

---

New commits:

883d507

Clean up PolynomialRing() constructor

[sagetrac-git]

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

2f8a591

Clean up PolynomialRing() constructor

[sagetrac-git]

Changed commit from 883d507 to 2f8a591

[sagetrac-git]

Changed commit from 2f8a591 to f0a314c

[sagetrac-git]

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

f0a314c

Clean up PolynomialRing() constructor

[sagetrac-git]

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

b410689

Clean up PolynomialRing() constructor

[sagetrac-git]

Changed commit from f0a314c to b410689

[tscrim]

comment:14

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.

[sagetrac-git]

Changed commit from b410689 to 98d9d59

[sagetrac-git]

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

98d9d59

Clean up PolynomialRing() constructor

[jdemeyer]

comment:16

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.

[sagetrac-git]

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

9be4fc3

Clean up PolynomialRing() constructor

[sagetrac-git]

Changed commit from 98d9d59 to 9be4fc3

[sagetrac-git]

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

dfb59b3

Clean up PolynomialRing() constructor

[sagetrac-git]

Changed commit from 9be4fc3 to dfb59b3

[jdemeyer]

comment:19

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

[tscrim]

comment:20

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.

[jdemeyer]

comment:21

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.

[jdemeyer]

comment:22

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.

[tscrim]

comment:23

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.

[tscrim]

comment:24

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.

[jdemeyer]

comment:25

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.

[sagetrac-git]

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

fc75734

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

[sagetrac-git]

Changed commit from dfb59b3 to fc75734

[tscrim]

comment:27

Thanks. LGTM.

[tscrim]

Reviewer: Travis Scrimshaw

[vbraun]

Changed branch from u/jdemeyer/clean_up_polynomial_constructor to fc75734