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:


The method "is_maximal" in sage/rings/ is broken:

sage: R = IntegerModRing(8)
sage: R.principal_ideal(0).is_maximal()
sage: R.principal_ideal(2).is_maximal()
sage: R.principal_ideal(4).is_maximal()

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)

trac_10934-is-maximal.patch (4.8 KB) - added by jhpalmieri 9 years ago.

Download all attachments as: .zip

Change History (4)

Changed 9 years ago by jhpalmieri

comment:1 Changed 9 years ago by jhpalmieri

  • Status changed from new to needs_review

Here's a patch: apply

  • trac_10934-is-maximal.patch

All tests pass for me on sage.math.

comment:2 Changed 9 years ago by spice

  • Authors set to John Palmieri
  • 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 jdemeyer

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