Opened 11 years ago
Closed 7 years ago
#8389 closed enhancement (fixed)
Implement MatrixSpace(...)['x']
Reported by:  mmezzarobba  Owned by:  AlexGhitza 

Priority:  minor  Milestone:  sage6.2 
Component:  algebra  Keywords:  
Cc:  mjo, nthiery  Merged in:  
Authors:  Marc Mezzarobba  Reviewers:  Nicolas M. Thiéry 
Report Upstream:  N/A  Work issues:  
Branch:  f4abe5d (Commits, )  Commit:  f4abe5d7ff07f6b7c2f83c8a0d993465ec755fed 
Dependencies:  Stopgaps: 
Description (last modified by )
...and rationalize the implementation of __getitem__
for rings.
Attachments (1)
Change History (24)
comment:1 Changed 11 years ago by
 Description modified (diff)
comment:2 Changed 10 years ago by
comment:3 Changed 9 years ago by
 Cc mjo added
 Status changed from new to needs_review
This is fixed, probably by #10470. I've added a doctest in the parent list() method; hopefully that is the proper place for it.
comment:4 followup: ↓ 5 Changed 9 years ago by
I was about to give this positive review, but after reading comment:2 I wonder. Are we just hiding a bug here? In which case this ticket could just be changed to either raising a NotImplementedError
or making it do what it's supposed to, namely creating a polynomial ring over the matrix space in question.
comment:5 in reply to: ↑ 4 Changed 9 years ago by
Replying to kcrisman:
I was about to give this positive review, but after reading comment:2 I wonder. Are we just hiding a bug here? In which case this ticket could just be changed to either raising a
NotImplementedError
or making it do what it's supposed to, namely creating a polynomial ring over the matrix space in question.
This is "easy" to do for one variable by overriding MatrixSpace_generic.__getitem__
to check for a string, and then calling PolynomialRing()
.
But ideally, we would want to offer the same interface that we do when constructing polynomial rings from other rings or fields. Does constructing a polynomial ring over matrix spaces even make sense mathematically? All of the existing code to do this is in ring.__getitem__
, which sounds right to me, but this is very much not a strong point of mine.
comment:6 Changed 9 years ago by
 Priority changed from major to minor
 Status changed from needs_review to needs_info
In any case, I think that this is at least 'needs info' until we decide what to do. It's not as high priority as it once was since it just raises an error instead of bringing the computer to a crashing halt!
comment:7 followup: ↓ 9 Changed 8 years ago by
There is (a duplicate) #10608 which has a patch that gives MatrixSpace_generic
a __getitem__()
method, so I think this should be fixed (i.e. return a polynomial ring). Also I don't think this hides a problem and is mathematically correct since all n x n
matrices form a ring (although not the nicest of rings). Plus PolynomialRing(MatrixSpace(QQ, 2), 'x')
works.
However what I'm thinking as a solution is that any parent in the category of Rings
should have a default __getitem__
which checks for string/list input and returns a polynomial/power series ring resp. Thoughts?
comment:8 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:9 in reply to: ↑ 7 Changed 7 years ago by
 Cc nthiery added
Replying to tscrim:
However what I'm thinking as a solution is that any parent in the category of
Rings
should have a default__getitem__
which checks for string/list input and returns a polynomial/power series ring resp. Thoughts?
In principle, I agree. Unfortunately, matrix spaces currently do not use the category framework by default (one needs to call M.full_category_initialisation()
first) for efficiency reasons. So the change you are suggesting would not solve the problem with matrix spaces by itself.
And I'm honestly at lost as to how to use the category framework with fundamental, widely used parents.
In our case, it would make sense (despite the issue with matrix rings) to move the definition of __getitem__
that deals with polynomials rings and the like from sage.structure.Rings
to sage.category.rings.Rings.ParentMethods
. But many common rings do not descend from Rings().parent_class
, so one would need a wrapper in one direction or the other. Since Rings.ParentMethods
is supposedly the recommended place to add generic stuff for rings in the long run, it would be natural to move the implementation there and provide a compatibility wrapper in Ring
. Except that Ring
comes before Rings.ParentMethods
in the MRO of (most?) rings that use both...
(On the top of that, there is a hack in Parent.__getitem__
that one needs to be careful not to break...)
comment:10 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:11 Changed 7 years ago by
 Branch set to u/mmezzarobba/8389ring_getitem
 Commit set to 8241fd65d1552db6255e911260cf6c3524c6ec8c
 Description modified (diff)
 Status changed from needs_info to needs_review
 Summary changed from Sage eats all memory trying to evaluate MatrixSpace(QQ, 2)['x'] to Implement MatrixSpace(...)['x']
Ok, after talking with Nicolas I think I understand better what is going on and what should be done. Here is an attempt to streamline the implementation of __getitem__
for general rings.
I didn't leave a version __getitem__
in ring.Ring
after all, because virtually all rings properly initialize their category by now. The doctests pass, but there is at least one ring (namely InfinityRing
) rings for which R['x']
will fail while it used to work, and there may be more. Thoughts?
New commits:
b446839  Improve Parent.__getitem__

286eb3c  Move __getitem__ from rings.ring to categories.ring

57b55c3  Move is_zero from Ring to Rings.ParentMethods

50b5381  Make gen_name private

f6a8daf  Simplify ZZ.__getitem__

0cf8043  Clean up and simplify Rings.ParentMethods.__getitem__

d3890c0  Further simplify/robustify Rings.ParentMethods.__getitem__

86dc8f9  A schizophrenic __getitem__ for matrix rings

0d960ec  Clarify that polynomials over general rings are supported

8241fd6  coercion tutorial: Update some examples...

comment:12 followup: ↓ 13 Changed 7 years ago by
Hi Mark!
I went through the changes, and overall it looks good! Thanks so much for the cleanup!
Some tiny remarks / suggestions:
 Doc of getitem
 TODO > .. TODO
 Is Frac nicer than .fraction_field()?
 Doc of _gen_names, line 2: replacing
ZZ['x']
byZZ[sqrt(5)]
could make it more meaninful?  getitem for a matrix space M: could
M[sqrt(5)]
be a meaningful notation to extend the base ring? If yes, I'd be in favor of completely deprecatingM[3]
in favor ofM.unrank(3)
, in order to eventually allow for the notationM[sqrt(5)]
without ambiguity in the corner caseM[1]
.
By the way: shall we use the occasion to also move PrincipalIdealDomain.__getitem__
in the corresponding category? Or are there some principal ideal domains in Sage that are not yet in the PrincipalIdealDomains
category?
Speaking of this method: its documentation says "Create a polynomial or power series ring over self
and inject the variables into the global module scope."; the latter statement is wrong, right?
Cheers,
Nicolas
comment:13 in reply to: ↑ 12 ; followup: ↓ 16 Changed 7 years ago by
Thanks for your review!
Replying to nthiery:
 Doc of getitem
 TODO > .. TODO
No, that was on purpose: the TODO was part of the SEEALSO block. But I added the missing crossreferences and removed the TODO line altogether. (I'll push the new commit in a moment.)
 Is Frac nicer than .fraction_field()?
No idea, I didn't touch this part :)
.
Let's mention both.
 Doc of _gen_names, line 2: replacing
ZZ['x']
byZZ[sqrt(5)]
could make it more meaninful?
Same thing here.
 getitem for a matrix space M: could
M[sqrt(5)]
be a meaningful notation to extend the base ring? If yes, I'd be in favor of completely deprecatingM[3]
in favor ofM.unrank(3)
, in order to eventually allow for the notationM[sqrt(5)]
without ambiguity in the corner caseM[1]
.
I believe M[sqrt(5)]
would make sense, and I wouldn't mind deprecating the notation parent[integer]
(as a way of enumerating the elements). But I'd rather do that in another ticket.
By the way: shall we use the occasion to also move
PrincipalIdealDomain.__getitem__
in the corresponding category? Or are there some principal ideal domains in Sage that are not yet in thePrincipalIdealDomains
category?Speaking of this method: its documentation says "Create a polynomial or power series ring over
self
and inject the variables into the global module scope."; the latter statement is wrong, right?
What method are you talking about?
Thanks again,
Marc
comment:14 Changed 7 years ago by
 Commit changed from 8241fd65d1552db6255e911260cf6c3524c6ec8c to d3e011961e08b5771eb16c78e6e5e31b42627311
Branch pushed to git repo; I updated commit sha1. New commits:
d3e0119  Improve doc of Rings.EM.__getitem__ and related functions

comment:15 Changed 7 years ago by
 Commit changed from d3e011961e08b5771eb16c78e6e5e31b42627311 to f4abe5d7ff07f6b7c2f83c8a0d993465ec755fed
Branch pushed to git repo; I updated commit sha1. New commits:
f4abe5d  Fix broken link in doc of Rings.EM.__getitem__

comment:16 in reply to: ↑ 13 ; followup: ↓ 17 Changed 7 years ago by
Replying to mmezzarobba:
No, that was on purpose: the TODO was part of the SEEALSO block. But I added the missing crossreferences and removed the TODO line altogether. (I'll push the new commit in a moment.)
 Is Frac nicer than .fraction_field()?
No idea, I didn't touch this part
:)
. Let's mention both.
 Doc of _gen_names, line 2: replacing
ZZ['x']
byZZ[sqrt(5)]
could make it more meaninful?Same thing here.
Ok. I double checked your changes and am happy with them!
I believe
M[sqrt(5)]
would make sense, and I wouldn't mind deprecating the notationparent[integer]
(as a way of enumerating the elements). But I'd rather do that in another ticket.
Fair enough :) Do you mind creating a ticket for this task?
By the way: shall we use the occasion to also move
PrincipalIdealDomain.__getitem__
in the corresponding category? Or are there some principal ideal domains in Sage that are not yet in thePrincipalIdealDomains
category?Speaking of this method: its documentation says "Create a polynomial or power series ring over
self
and inject the variables into the global module scope."; the latter statement is wrong, right?
What method are you talking about?
Sorry, I confused myself with Ring.getitem from another branch ...
Btw: what do you think we should do with IntegerRing?.getitem which calls explicitly PrincipalIdealDomains?.getitem? Just leave it as it is?
Cheers,
Nicolas
comment:17 in reply to: ↑ 16 ; followup: ↓ 18 Changed 7 years ago by
Replying to nthiery:
I believe
M[sqrt(5)]
would make sense, and I wouldn't mind deprecating the notationparent[integer]
(as a way of enumerating the elements). But I'd rather do that in another ticket.Fair enough :) Do you mind creating a ticket for this task?
Done (#15885).
Btw: what do you think we should do with IntegerRing?.getitem which calls explicitly PrincipalIdealDomains?.getitem? Just leave it as it is?
For now yes.
Can you please set the ticket to positive review if you are happy with it?
comment:18 in reply to: ↑ 17 Changed 7 years ago by
Replying to mmezzarobba:
Done (#15885).
Thanks!
For now yes.
Ok.
Can you please set the ticket to positive review if you are happy with it?
Done!
comment:19 Changed 7 years ago by
 Status changed from needs_review to positive_review
comment:20 Changed 7 years ago by
Oh, by the way, should this be a defect fix or an enhancement?
comment:21 Changed 7 years ago by
 Reviewers set to Nicolas M. Thiéry
comment:22 Changed 7 years ago by
 Type changed from defect to enhancement
comment:23 Changed 7 years ago by
 Branch changed from u/mmezzarobba/8389ring_getitem to f4abe5d7ff07f6b7c2f83c8a0d993465ec755fed
 Resolution set to fixed
 Status changed from positive_review to closed
The problem's not in the polynomial ring constructor per se:
Almost nothing works with R because printing of elements is broken, but at least you can construct it!
The problem reported above lies in the
R['x']
syntax; for some reason, the "list" method of the matrix ring is getting called, and this (of course) never terminates. If you try this with a matrix space over a *finite* ring, the list call succeeds, and it tries to get the element of index 'x'  which fails because the string 'x' isn't an integer: