#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: |
Description
as another small step towards py3
Change History (28)
comment:1 Changed 5 years ago by
- Branch set to u/chapoton/23787
- Commit set to f227266892a33010320a9b785dcd096d5b7f5c77
- Status changed from new to needs_info
comment:2 Changed 5 years ago by
Typically, one would replace __cmp__
by __richcmp__
, not _richcmp_
.
comment:3 Changed 5 years ago by
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
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
PS, this will conflict with changes on #22982. However, I suspect this will be ready first.
comment:6 Changed 5 years ago by
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
- Commit changed from f227266892a33010320a9b785dcd096d5b7f5c77 to b0a886665b973d75a1e476860ddc33498d193134
comment:8 Changed 5 years ago by
- Status changed from needs_info to needs_work
comment:9 Changed 5 years ago by
This is now the last set of places where cmp is used.
comment:10 Changed 5 years ago by
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: ↓ 12 Changed 5 years ago by
In number field ideals, why add .sage()
to the PARI instances?
comment:12 in reply to: ↑ 11 Changed 5 years ago by
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
Hmm. It seems that MPolynomialIdeal uses two different semantics for __lt__
and __gt___
(for inclusion) and __cmp__
.
comment:14 Changed 5 years ago by
I have split-off #23920
comment:15 Changed 5 years ago by
- Commit changed from b0a886665b973d75a1e476860ddc33498d193134 to 5d2c453759ce8cb1f73e5f871b449bdc101153d9
Branch pushed to git repo; I updated commit sha1. New commits:
5d2c453 | Merge branch 'u/chapoton/23787' in 8.1.b8
|
comment:16 Changed 5 years ago by
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
- Cc jdemeyer tscrim jhpalmieri added
Jeroen, any idea for my previous question, please ?
comment:18 follow-up: ↓ 19 Changed 5 years ago by
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
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
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
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
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
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
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
- 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:
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.
|
comment:26 Changed 5 years ago by
- 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
- 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
- Commit b003684f2eead00ca678a8b27db0235a64396d0a deleted
Scrimsaw sounds cool though :-)
some doctests are failing, needs to be investigated
New commits:
py3: rich comparison of ideals