Opened 11 years ago

Closed 7 years ago

#8389 closed enhancement (fixed)

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

Reported by: mmezzarobba Owned by: AlexGhitza
Priority: minor Milestone: sage-6.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:

Status badges

Description (last modified by mmezzarobba)

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

Attachments (1)

sage-trac_8389.patch (957 bytes) - added by mjo 9 years ago.
Add a doctest to the parent list() method.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 11 years ago by mmezzarobba

  • Description modified (diff)

comment:2 Changed 10 years ago by davidloeffler

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

comment:3 Changed 9 years ago by mjo

  • Authors set to Michael Orlitzky
  • 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.

Changed 9 years ago by mjo

Add a doctest to the parent list() method.

comment:4 follow-up: Changed 9 years ago by 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.

comment:5 in reply to: ↑ 4 Changed 9 years ago by mjo

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 kcrisman

  • 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 follow-up: Changed 8 years ago by tscrim

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 jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:9 in reply to: ↑ 7 Changed 7 years ago by mmezzarobba

  • 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 vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:11 Changed 7 years ago by mmezzarobba

  • Authors changed from Michael Orlitzky to Marc Mezzarobba
  • Branch set to u/mmezzarobba/8389-ring_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:

b446839Improve Parent.__getitem__
286eb3cMove __getitem__ from rings.ring to categories.ring
57b55c3Move is_zero from Ring to Rings.ParentMethods
50b5381Make gen_name private
f6a8dafSimplify ZZ.__getitem__
0cf8043Clean up and simplify Rings.ParentMethods.__getitem__
d3890c0Further simplify/robustify Rings.ParentMethods.__getitem__
86dc8f9A schizophrenic __getitem__ for matrix rings
0d960ecClarify that polynomials over general rings are supported
8241fd6coercion tutorial: Update some examples...

comment:12 follow-up: Changed 7 years ago by nthiery

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

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by mmezzarobba

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

comment:14 Changed 7 years ago by git

  • Commit changed from 8241fd65d1552db6255e911260cf6c3524c6ec8c to d3e011961e08b5771eb16c78e6e5e31b42627311

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

d3e0119Improve doc of Rings.EM.__getitem__ and related functions

comment:15 Changed 7 years ago by git

  • Commit changed from d3e011961e08b5771eb16c78e6e5e31b42627311 to f4abe5d7ff07f6b7c2f83c8a0d993465ec755fed

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

f4abe5dFix broken link in doc of Rings.EM.__getitem__

comment:16 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by nthiery

Replying to mmezzarobba:

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

comment:17 in reply to: ↑ 16 ; follow-up: Changed 7 years ago by mmezzarobba

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?

comment:18 in reply to: ↑ 17 Changed 7 years ago by nthiery

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 nthiery

  • Status changed from needs_review to positive_review

comment:20 Changed 7 years ago by nthiery

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

comment:21 Changed 7 years ago by vbraun

  • Reviewers set to Nicolas M. Thiéry

comment:22 Changed 7 years ago by mmezzarobba

  • Type changed from defect to enhancement

comment:23 Changed 7 years ago by vbraun

  • Branch changed from u/mmezzarobba/8389-ring_getitem to f4abe5d7ff07f6b7c2f83c8a0d993465ec755fed
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.