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: |
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)
Change History (11)
comment:1 Changed 13 years ago by
- Status changed from new to needs_review
comment:2 Changed 13 years ago by
- Status changed from needs_review to needs_info
comment:3 Changed 13 years ago by
- 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
- 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
comment:5 Changed 13 years ago by
- 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
- 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.
comment:7 Changed 13 years ago by
- Status changed from needs_review to positive_review
comment:8 Changed 13 years ago by
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
- Merged in set to sage-4.3.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
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.