Implement MatrixSpace(...)['x']

[mezzarobba]

...and rationalize the implementation of __getitem__ for rings.

CC: @orlitzky @nthiery

Component: algebra

Author: Marc Mezzarobba

Branch/Commit: f4abe5d

Reviewer: Nicolas M. Thiéry

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

[loefflerd]

comment:2

The problem's not in the polynomial ring constructor per se:

sage: R = PolynomialRing(MatrixSpace(QQ, 2),'x'); R
Univariate Polynomial Ring in x over Full MatrixSpace of 2 by 2 dense matrices over Rational Field

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:

sage: MatrixSpace(GF(3), 2)['x']
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/masiao/<ipython console> in <module>()

/storage/masiao/sage-4.6.2.alpha1/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.__getitem__ (sage/structure/parent.c:8072)()

TypeError: list indices must be integers, not str

[orlitzky]

Author: Michael Orlitzky

[orlitzky]

comment:3

This is fixed, probably by #10470. I've added a doctest in the parent list() method; hopefully that is the proper place for it.

[orlitzky]

Attachment: sage-trac_8389.patch.gz

Add a doctest to the parent list() method.

[kcrisman]

comment:4

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.

[orlitzky]

comment:5

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.

[kcrisman]

comment:6

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!

[tscrim]

comment:7

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?

[mezzarobba]

comment:9

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

[mezzarobba]

Commit: 8241fd6

[mezzarobba]

Changed author from Michael Orlitzky to Marc Mezzarobba

[mezzarobba]

comment:11

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

[mezzarobba]

Branch: u/mmezzarobba/8389-ring_getitem

[nthiery]

comment:12

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'] by ZZ[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 deprecating M[3] in favor of M.unrank(3), in order to eventually allow for the notation M[sqrt(5)] without ambiguity in the corner case M[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

[mezzarobba]

comment:13

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 cross-references 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'] by ZZ[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 deprecating M[3] in favor of M.unrank(3), in order to eventually allow for the notation M[sqrt(5)] without ambiguity in the corner case M[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 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?

What method are you talking about?

Thanks again,

Marc

[sagetrac-git]

Changed commit from 8241fd6 to d3e0119

[sagetrac-git]

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

d3e0119

Improve doc of Rings.EM.__getitem__ and related functions

[sagetrac-git]

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

f4abe5d

Fix broken link in doc of Rings.EM.__getitem__

[sagetrac-git]

Changed commit from d3e0119 to f4abe5d

[nthiery]

comment:16

Replying to @mezzarobba:

No, that was on purpose: the TODO was part of the SEEALSO block. But I added the missing cross-references 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'] by ZZ[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 notation parent[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 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?

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

[mezzarobba]

comment:17

Replying to @nthiery:

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.

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?

[nthiery]

comment:18

Replying to @mezzarobba:

Done (#15885).

Thanks!

For now yes.

Ok.

Can you please set the ticket to positive review if you are happy with it?

Done!

[nthiery]

comment:20

Oh, by the way, should this be a defect fix or an enhancement?

[vbraun]

Reviewer: Nicolas M. Thiéry

[vbraun]

Changed branch from u/mmezzarobba/8389-ring_getitem to f4abe5d