improvements to quotient_ring.py

[aghitza]

The attached patch makes a few improvements in rings/quotient_ring.py: implement is_noetherian, fix a todo in cover_ring, and minor docstring fixes.

Component: algebra

Author: Alex Ghitza

Reviewer: John Palmieri

Merged: sage-4.3.alpha1

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

[jhpalmieri]

comment:2

Can self.cover_ring() ever be non-Noetherian (with our current code)?

Of course if you take an infinitely generated polynomial ring over a field and mod out by all of the polynomial generators, the quotient is Noetherian but the cover ring is not. I don't know if that can come up in Sage now, and I also don't know how else we would easily tell whether the quotient ring is Noetherian...

Otherwise, the patch looks good.

[aghitza]

comment:3

That's a very good point, I should definitely fix this. I'll have is_noetherian return True if cover_ring is Noetherian, and raise a NotImplementedError otherwise.

I'm marking this as needs_work, and I'll try to get to it soon.

[aghitza]

comment:4

Alright, in the process of fixing this I added a few trivial methods to InfinitePolynomialRing and fixed a NotImplementedError being returned rather than raised in ring.pyx.

[aghitza]

Reviewer: John Palmieri

[aghitza]

Attachment: trac_7457.patch.gz

[jhpalmieri]

comment:5

Two comments: this is not your fault, but can you fix lines 39-40 of quotient_ring.py? Right now it says

    Creates a quotient ring of the ring `R` by the ideal `I`. Variables are 
    labeled by ``names``. (If the quotient ring is a quotient of a 
    polynomial ring.). If ``names`` isn't given, 'bar' will be appended to 
    the variable names in `R`. 

and the parentheses and surrounding punctuation really bother me. Should this say

    Creates a quotient ring of the ring `R` by the ideal `I`. Variables are 
    labeled by ``names`` (if the quotient ring is a quotient of a 
    polynomial ring). If ``names`` isn't given, 'bar' will be appended to 
    the variable names in `R`. 

? Or even remove the parentheses altogether?

Second and more importantly, I'm getting doctest failures in schemes/elliptic_curves:

The following tests failed:

	sage -t -long devel/sage/sage/schemes/elliptic_curves/sha_tate.py # 8 doctests failed
	sage -t -long devel/sage/sage/schemes/elliptic_curves/padics.py # 46 doctests failed
	sage -t -long devel/sage/sage/schemes/elliptic_curves/formal_group.py # 1 doctests failed
	sage -t -long devel/sage/sage/schemes/elliptic_curves/monsky_washnitzer.py # 3 doctests failed

The problem seems to be the change to rings.pyx. I don't know why you would ever want to return a NotImplementedError instead of raising it, but that change seems to be causing the problems. So my suggestion is to get rid of that change, make sure doctests pass, and then perhaps open up a new ticket in which that issue is addressed.

[jhpalmieri]

comment:6

Here's a referee's patch to fix these two issues. See #7532 for a follow-up.

[jhpalmieri]

Attachment: trac_7457-ref.patch.gz

apply on top of the other patch

[aghitza]

comment:8

Wow, I look away for a couple of days and this is all done and fixed. Thanks, John!

[mwhansen]

Merged: sage-4.3.alpha1