py3: richcmp for ideals

[chapoton]

as another small step towards py3

[chapoton]

some doctests are failing, needs to be investigated

---

New commits:

​f227266

py3: rich comparison of ideals

[jdemeyer]

Typically, one would replace __cmp__ by __richcmp__, not _richcmp_.

[chapoton]

This is work in progress.. But one has

class Ideal_generic(MonoidElement)

so every ideal is an "Element"...

[tscrim]

It looks like all of the failures are related to the ordering of the ideals. You have introduced at least one behavior change: Principal ideals used to be considered as smaller as non-principal ideals, but now that no longer holds on your branch. So this should fix the test failure in rings/ideal.py. I suspect the others are coming from your changes in rings/number_field/number_field_ideal.py.

[tscrim]

PS, this will conflict with changes on #22982. However, I suspect this will be ready first.

[chapoton]

I suspect that this ticket here will be hard to fix. If it was easy, this would not be my third or 4th failing tentative to solve the issue.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​1815de6	Merge branch 'u/chapoton/23787' in 8.1.b5
​b0a8866	trac 23787 detail

[chapoton]

This is now the last set of places where cmp is used.

[jdemeyer]

I think you need to decide whether you want __richcmp__ or _richcmp_ here. I understand why _richcmp_ makes sense, but then you should also remove the various isinstance() checks that are still there.

[jdemeyer]

In number field ideals, why add .sage() to the PARI instances?

[chapoton]

Replying to jdemeyer:

In number field ideals, why add .sage() to the PARI instances?

Because the pari objects could not be compared. If you know a better solution, please go on.

[chapoton]

Hmm. It seems that MPolynomialIdeal uses two different semantics for __lt__ and __gt___ (for inclusion) and __cmp__.

[chapoton]

I have split-off #23920

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​5d2c453

Merge branch 'u/chapoton/23787' in 8.1.b8

[chapoton]

Jeroen, is there a way to change the following behaviour (cmp works but not richcmp on cypari Gen objects) ?

sage: from sage.structure.richcmp import *
sage: K.<a, b> = NumberField([x^2 + 23, x^2 - 7])
sage: I = K.ideal(2, (a + 2*b + 3)/2)
sage: J = K.ideal(2, a - b)
sage: richcmp(I.pari_rhnf(), J.pari_rhnf() , op_LE)
---------------------------------------------------------------------------
PariError                                 Traceback (most recent call last)
<ipython-input-24-c3ae4cea545c> in <module>()
----> 1 richcmp(I.pari_rhnf(), J.pari_rhnf() , op_LE)

/home/chapoton/sage/src/sage/structure/richcmp.pyx in sage.structure.richcmp.richcmp (build/cythonized/sage/structure/richcmp.c:1361)()
    103         False
    104     """
--> 105     return PyObject_RichCompare(x, y, op)
    106 
    107 

cypari2/gen.pyx in cypari2.gen.Gen.__richcmp__()

cypari2/handle_error.pyx in cypari2.handle_error._pari_err_handle()

PariError: forbidden comparison t_VEC (2 elts) , t_VEC (2 elts)
sage: cmp(I.pari_rhnf(), J.pari_rhnf())
-1

[chapoton]

Jeroen, any idea for my previous question, please ?

[chapoton]

This ticket (last call to cmp anywhere) seem to be stuck..

The current branch has only 4 very mild failing doctests, that I have failed to understand and fix.

Opinion required, please. Should I just fix the doctests, or should one try to match absolutely the existing behaviour ?

[tscrim]

Replying to chapoton:

This ticket (last call to cmp anywhere) seem to be stuck..

Wow, that is amazing. Thank you for all your work so far. Now to get over this.

The current branch has only 4 very mild failing doctests, that I have failed to understand and fix.

I will look into them today. Although I don't know anything about comment:16.

Opinion required, please. Should I just fix the doctests, or should one try to match absolutely the existing behaviour ?

I think we should try to match the existing behavior, or at least understand why the changes are necessary. The tests that have a different order are not so big IMO, but the I < J and J > I test failing should be explained.

[tscrim]

The change in the output for the SUK = UnitGroup(K,S=21); SUK test comes down to sorting in the factorization object.

[tscrim]

The cmp calls Gen.__cmp__ calls PARI's cmp_universal, which has no mathematical meaning ​according to the docs. So converting back to Sage objects and doing the comparison is a much better comparison IMO. Subsequently that is a test we can simply update.

[tscrim]

The test in Ideal_principal._richcmp_ does not actually test that code:

sage: I.__class__.__mro__
(<class 'sage.rings.polynomial.multi_polynomial_ideal.MPolynomialIdeal'>,
 <class sage.rings.polynomial.multi_polynomial_ideal.MPolynomialIdeal_singular_repr at 0x7f98c6dbc668>,
 <class sage.rings.polynomial.multi_polynomial_ideal.MPolynomialIdeal_singular_base_repr at 0x7f98c6dbc600>,
 <class sage.rings.polynomial.multi_polynomial_ideal.MPolynomialIdeal_macaulay2_repr at 0x7f98c6dbc6d0>,
 <class sage.rings.polynomial.multi_polynomial_ideal.MPolynomialIdeal_magma_repr at 0x7f98c6dbc598>,
 <class 'sage.rings.ideal.Ideal_generic'>,
 <type 'sage.structure.element.MonoidElement'>,
 <type 'sage.structure.element.Element'>,
 <type 'sage.structure.sage_object.SageObject'>,
 <type 'object'>)

However, all the same, that is not the problem. Something very strange is going on:

sage: [richcmp(I.gens(), J.gens(), val) for val in range(6)]  # Defines the behavior
[False, False, False, True, True, True]
sage: [richcmp(I, J, val) for val in range(6)]  # What?
[True, True, False, True, False, False]
sage: [I._richcmp_(J, val) for val in range(6)]  # What!?!?!?
[False, False, False, True, True, True]

[tscrim]

Oh, I know why. Just look at I.__richcmp__, which gives the multivariate comparison (using Gröbner bases) and never even sees this code path. So I am going to remove the I > J and J < I tests.

[tscrim]

Correction, I am going to change the test to not use the multivariate code and actually test that code.

[tscrim]

Okay, I fixed that and a bug along the way now that the code is actually being tested.

The remaining failure is the same as the other number field because it was using cmp_universal. So I just updated the output.

Needs review.

---

New commits:

​4a0ed9c	Merge branch 'u/chapoton/23787' of git://trac.sagemath.org/sage into public/python3/richcmp_ideals-23787
​d183891	Updating unit_group.py test.
​5b6624a	Actually test the _richcmp_ code and fixing a bug.
​b003684	Updating test because the ordering of ideals changed.

[chapoton]

Thanks a lot. The bot is morally green, so I am going to set this to positive review.

[jdemeyer]

Scrimsaw sounds cool though :-)