is_maximal is broken

[jhpalmieri]

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.

Component: algebra

Keywords: maximal ideal

Author: John Palmieri

Reviewer: Simon Spicer

Merged: sage-4.7.alpha5

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

[jhpalmieri]

comment:1

Attachment: trac_10934-is-maximal.patch.gz

Here's a patch: apply

• trac_10934-is-maximal.patch

All tests pass for me on sage.math.

[haikona]

Author: John Palmieri

[haikona]

Reviewer: Simon Spicer

[haikona]

comment:2

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?

[jdemeyer]

Merged: sage-4.7.alpha5