Opened 23 months ago
Closed 22 months ago
#23338 closed enhancement (fixed)
Clean up polynomial constructor
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.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 )
This ticket cleans up the PolynomialRing()
function in many ways. Mainly:
 Use
normalize_names()
as much as possible to deal with variable names.
 Fix indentation in docstring.
 Pass arguments like
sparse
andimplementation
as**kwds
to the singlevariate or multivariate polynomial constructor.
 Make the code easier to understand.
 Allow
PolynomialRing(..., implementation="singular")
which forces Singular. This allows simplifying some code insrc/sage/algebras/free_algebra.py
which currently needs to jump through hoops to get aMPolynomialRing_libsingular
.
 Check more error conditions, for example currently we have
sage: PolynomialRing(QQ, name="x", names="y") Univariate Polynomial Ring in y over Rational Field
 Change some arguments of
PolynomialRing()
to keywordonly arguments. This does break some existing uses ofPolynomialRing()
, although much more likely in library code and not user code (some code in Sage gets this even wrong and was passingsparse="lex"
instead oforder="lex"
)
Apart from items 6 and 7, no existing functionality is changed.
Change History (28)
comment:1 Changed 23 months ago by
 Cc tscrim added
comment:2 Changed 23 months ago by
 Branch set to u/jdemeyer/clean_up_polynomial_constructor
comment:3 Changed 23 months ago by
 Commit set to 883d50709298487518d7d0a2771045483adb0cf9
First version, not finished or tested yet. But feel free to comment...
New commits:
883d507  Clean up PolynomialRing() constructor

comment:4 Changed 23 months ago by
 Description modified (diff)
comment:5 Changed 23 months ago by
 Description modified (diff)
comment:6 Changed 23 months ago by
 Description modified (diff)
comment:7 Changed 23 months ago by
 Description modified (diff)
comment:8 Changed 23 months ago by
 Description modified (diff)
comment:9 Changed 23 months ago by
 Description modified (diff)
comment:10 Changed 23 months ago by
 Commit changed from 883d50709298487518d7d0a2771045483adb0cf9 to 2f8a591a6b7151c442c9d20b53a5131eaa26e3b0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2f8a591  Clean up PolynomialRing() constructor

comment:11 Changed 23 months ago by
 Commit changed from 2f8a591a6b7151c442c9d20b53a5131eaa26e3b0 to f0a314c800e0f060008a9402d1a7f2fba968920b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f0a314c  Clean up PolynomialRing() constructor

comment:12 Changed 23 months ago by
 Description modified (diff)
comment:13 Changed 23 months ago by
 Commit changed from f0a314c800e0f060008a9402d1a7f2fba968920b to b410689bf2e5ef76ff9010f541a070d71778484f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b410689  Clean up PolynomialRing() constructor

comment:14 followup: ↓ 16 Changed 23 months ago by
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 thatkwnames != 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 novar_array
block.  We probably can/should handle the
R.<x> = PolynomialRing(QQ, 'x')
input entirely in the novar_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 forvar_array
input.
comment:15 Changed 23 months ago by
 Commit changed from b410689bf2e5ef76ff9010f541a070d71778484f to 98d9d596260ccb224fcc565693b733b437dbab7a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
98d9d59  Clean up PolynomialRing() constructor

comment:16 in reply to: ↑ 14 ; followup: ↓ 20 Changed 23 months ago by
Replying to tscrim:
 In handling the case
R.<x> = PolynomialRing(QQ, 'x')
, you should normalize the names there so thatkwnames != 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 novar_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 forvar_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
 Commit changed from 98d9d596260ccb224fcc565693b733b437dbab7a to 9be4fc3ef59303385f7d5489e970441dfe81a212
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9be4fc3  Clean up PolynomialRing() constructor

comment:18 Changed 23 months ago by
 Commit changed from 9be4fc3ef59303385f7d5489e970441dfe81a212 to dfb59b3f87dac632ed040afceb0340800f8ceb6e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
dfb59b3  Clean up PolynomialRing() constructor

comment:19 Changed 23 months ago by
 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 ; followup: ↓ 21 Changed 23 months ago by
Replying to jdemeyer:
Replying to tscrim:
 In handling the case
R.<x> = PolynomialRing(QQ, 'x')
, you should normalize the names there so thatkwnames != 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 callnormalize_names()
onkwnames
.
Ah, I see. Okay.
 You also almost already check for
R.<x> = PolynomialRing(QQ, 'x')
in the novar_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 likeR.<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 forvar_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 thePolynomialRing
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
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
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 1variable case properly. So I feel inclined to keep the current behaviour.
comment:23 Changed 23 months ago by
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 followup: ↓ 25 Changed 23 months ago by
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
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 inpolynomial_singular_interface.py
andinterfaces/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
 Commit changed from dfb59b3f87dac632ed040afceb0340800f8ceb6e to fc75734d30afcbe5604ff835ae2d1bb5b983e713
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

comment:27 Changed 23 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Thanks. LGTM.
comment:28 Changed 22 months ago by
 Branch changed from u/jdemeyer/clean_up_polynomial_constructor to fc75734d30afcbe5604ff835ae2d1bb5b983e713
 Resolution set to fixed
 Status changed from positive_review to closed
+1