Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#23787 closed enhancement (fixed)

py3: richcmp for ideals

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.1
Component: python3 Keywords:
Cc: jdemeyer, tscrim, jhpalmieri Merged in:
Authors: Frédéric Chapoton, Travis Scrimshaw Reviewers: Travis Scrimshaw, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: b003684 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

as another small step towards py3

Change History (28)

comment:1 Changed 5 years ago by chapoton

  • Branch set to u/chapoton/23787
  • Commit set to f227266892a33010320a9b785dcd096d5b7f5c77
  • Status changed from new to needs_info

some doctests are failing, needs to be investigated


New commits:

f227266py3: rich comparison of ideals

comment:2 Changed 5 years ago by jdemeyer

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

comment:3 Changed 5 years ago by chapoton

This is work in progress.. But one has

class Ideal_generic(MonoidElement)

so every ideal is an "Element"...

comment:4 Changed 5 years ago by 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.

comment:5 Changed 5 years ago by tscrim

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

comment:6 Changed 5 years ago by 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.

comment:7 Changed 5 years ago by git

  • Commit changed from f227266892a33010320a9b785dcd096d5b7f5c77 to b0a886665b973d75a1e476860ddc33498d193134

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

1815de6Merge branch 'u/chapoton/23787' in 8.1.b5
b0a8866trac 23787 detail

comment:8 Changed 5 years ago by chapoton

  • Status changed from needs_info to needs_work

comment:9 Changed 5 years ago by chapoton

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

comment:10 Changed 5 years ago by 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.

comment:11 follow-up: Changed 5 years ago by jdemeyer

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

comment:12 in reply to: ↑ 11 Changed 5 years ago by 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.

comment:13 Changed 5 years ago by chapoton

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

comment:14 Changed 5 years ago by chapoton

I have split-off #23920

comment:15 Changed 5 years ago by git

  • Commit changed from b0a886665b973d75a1e476860ddc33498d193134 to 5d2c453759ce8cb1f73e5f871b449bdc101153d9

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

5d2c453Merge branch 'u/chapoton/23787' in 8.1.b8

comment:16 Changed 5 years ago by 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

comment:17 Changed 5 years ago by chapoton

  • Cc jdemeyer tscrim jhpalmieri added

Jeroen, any idea for my previous question, please ?

comment:18 follow-up: Changed 5 years ago by 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 ?

comment:19 in reply to: ↑ 18 Changed 5 years ago by 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.

comment:20 Changed 5 years ago by tscrim

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

comment:21 Changed 5 years ago by 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.

comment:22 Changed 5 years ago by 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]

comment:23 Changed 5 years ago by 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.

comment:24 Changed 5 years ago by tscrim

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

comment:25 Changed 5 years ago by tscrim

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, Travis Scrimsaw
  • Branch changed from u/chapoton/23787 to public/python3/richcmp_ideals-23787
  • Commit changed from 5d2c453759ce8cb1f73e5f871b449bdc101153d9 to b003684f2eead00ca678a8b27db0235a64396d0a
  • Reviewers set to Travis Scrimshaw, Frédéric Chapoton
  • Status changed from needs_work to needs_review

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:

4a0ed9cMerge branch 'u/chapoton/23787' of git://trac.sagemath.org/sage into public/python3/richcmp_ideals-23787
d183891Updating unit_group.py test.
5b6624aActually test the _richcmp_ code and fixing a bug.
b003684Updating test because the ordering of ideals changed.

comment:26 Changed 5 years ago by chapoton

  • Status changed from needs_review to positive_review

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

comment:27 Changed 5 years ago by vbraun

  • Branch changed from public/python3/richcmp_ideals-23787 to b003684f2eead00ca678a8b27db0235a64396d0a
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:28 Changed 5 years ago by jdemeyer

  • Authors changed from Frédéric Chapoton, Travis Scrimsaw to Frédéric Chapoton, Travis Scrimshaw
  • Commit b003684f2eead00ca678a8b27db0235a64396d0a deleted

Scrimsaw sounds cool though :-)

Note: See TracTickets for help on using tickets.