Opened 9 years ago
Closed 9 years ago
#10934 closed defect (fixed)
is_maximal is broken
Reported by: | jhpalmieri | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7 |
Component: | algebra | Keywords: | maximal ideal |
Cc: | Merged in: | sage-4.7.alpha5 | |
Authors: | John Palmieri | Reviewers: | Simon Spicer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
The method "is_maximal" in sage/rings/ideal.py is broken:
sage: R = IntegerModRing(8) sage: R.principal_ideal(0).is_maximal() True sage: R.principal_ideal(2).is_maximal() True sage: R.principal_ideal(4).is_maximal() True
Admittedly, the docstring does say
TODO: Make self.is_maximal() work! Write this code!
but we need a trac ticket for this. The comments in the code are not right, either:
kd = self.ring().krull_dimension() if kd == 0: # every non-trivial ideal is maximal
This appears to be false, as the example above (Z/8Z) illustrates. It would be true if the ring were an integral domain (because Krull dimension 0 + integral domain = field). Alternatively, it would be true if the ideal were prime, but we have limited primality testing right now, so this is not the best way to go: with R as above, R.principal_ideal(2).is_prime()
raises a NotImplementedError
. Next:
elif kd == 1 and self.ring().is_integral_domain(): # every nontrivial ideal is maximal return self.is_prime()
The comment should say "every nontrivial prime ideal is maximal, so the code is right but the comment isn't.
Attachments (1)
Change History (4)
Changed 9 years ago by
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Reviewers set to Simon Spicer
- Status changed from needs_review to positive_review
All tests pass for me too, and the code behaves as intended on all the examples I could think of. Positive review.
Since this is only a partial implementation of is_maximal() i.e. only for quotient rings of ZZ
, should we open a separate ticket for implementing this over general rings? Or does such a ticket already exist?
comment:3 Changed 9 years ago by
- Merged in set to sage-4.7.alpha5
- Resolution set to fixed
- Status changed from positive_review to closed
Here's a patch: apply
All tests pass for me on sage.math.