Opened 13 years ago

Closed 13 years ago

#7457 closed enhancement (fixed)

improvements to quotient_ring.py

Reported by: AlexGhitza Owned by: AlexGhitza
Priority: minor Milestone: sage-4.3
Component: algebra Keywords:
Cc: Merged in: sage-4.3.alpha1
Authors: Alex Ghitza Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

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.

Attachments (2)

trac_7457.patch (9.2 KB) - added by AlexGhitza 13 years ago.
trac_7457-ref.patch (1.2 KB) - added by jhpalmieri 13 years ago.
apply on top of the other patch

Download all attachments as: .zip

Change History (11)

comment:1 Changed 13 years ago by AlexGhitza

  • Status changed from new to needs_review

comment:2 Changed 13 years ago by jhpalmieri

  • Status changed from needs_review to needs_info

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.

comment:3 Changed 13 years ago by AlexGhitza

  • Status changed from needs_info to needs_work

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.

comment:4 Changed 13 years ago by AlexGhitza

  • Reviewers set to John Palmieri
  • Status changed from needs_work to needs_review

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.

Changed 13 years ago by AlexGhitza

comment:5 Changed 13 years ago by jhpalmieri

  • Report Upstream set to N/A
  • Status changed from needs_review to needs_work

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.

comment:6 Changed 13 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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

Changed 13 years ago by jhpalmieri

apply on top of the other patch

comment:7 Changed 13 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

comment:8 Changed 13 years ago by AlexGhitza

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

comment:9 Changed 13 years ago by mhansen

  • Merged in set to sage-4.3.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.