Opened 3 years ago
Closed 3 years ago
#22048 closed enhancement (fixed)
py3 remove cmp() in number_field/number_field_element (pyx)
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.6 |
Component: | python3 | Keywords: | |
Cc: | tscrim, jdemeyer, aapitzsch | Merged in: | |
Authors: | Frédéric Chapoton | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | a943b9a (Commits) | Commit: | a943b9ab0b07e5d2bbccc9cfea6523c37b143fce |
Dependencies: | Stopgaps: |
Description
as a step towards correct cythonization and py3
Change History (21)
comment:1 Changed 3 years ago by
- Branch set to u/chapoton/22048
- Cc tscrim jdemeyer aapitzsch added
- Commit set to b91273eb1952577d423e8b2b689a5b7030b741f0
- Status changed from new to needs_review
comment:2 Changed 3 years ago by
- Status changed from needs_review to needs_info
What is the point of removing cmp()
while keeping __cmp__
?
comment:3 follow-up: ↓ 4 Changed 3 years ago by
comment:4 in reply to: ↑ 3 Changed 3 years ago by
I think it makes little sense to change the same comparison function twice: once to get rid of cmp()
and once to change __cmp__
to __richcmp__
. That seems like more work without much reason.
Replying to chapoton:
my current aim is to be able to cythonize all our pyx files.
If that is really your goal, I would just implement a "fake" cmp()
function like this:
cdef int cmp(x, y) except -2: if x == y: return 0 elif x < y: return -1 else: return 1
comment:5 follow-up: ↓ 11 Changed 3 years ago by
I would prefer to avoid re-creating cmp, but I can bend if it is required.
In which file should I put this "custom cmp" function ?
comment:6 Changed 3 years ago by
I tried to used __richcmp__
here, but this breaks pickling. Do you have any idea why ? Maybe something like "Non-identical instances of a class normally compare as non-equal unless the class defines the __eq__()
method." ? Can this be solved ?
********************************************************************** File "src/sage/rings/number_field/number_field_element.pyx", line 5000, in sage.rings.number_field.number_field_element.CoordinateFunction Failed example: f == loads(dumps(f)) Expected: True Got: False ********************************************************************** File "src/sage/rings/number_field/number_field_element.pyx", line 5068, in sage.rings.number_field.number_field_element.CoordinateFunction.__richcmp__ Failed example: f == loads(dumps(f)) Expected: True Got: False
comment:7 Changed 3 years ago by
Can you push your branch where you do __richcmp__
(not necessarily setting the branch field of this ticket of course)?
comment:8 Changed 3 years ago by
a branch with richmp is available at u/chapoton/experiment_22048
comment:9 follow-up: ↓ 10 Changed 3 years ago by
So my guess is that Cython tries to find a __cmp__
, and if it doesn't, then (unlike Python) does not use __richcmp__
. For proper Element
classes, then there is a default __cmp__
, which redirects appropriately to _richcmp_
.
However, I'm not sure if this could be considered a bug of Cython, if it something we just have to work around (temporarily until Cython gets this as a feature), or we just have __cmp__
in Cython classes because it is a different language. Jeroen, any comments?
comment:10 in reply to: ↑ 9 Changed 3 years ago by
Replying to tscrim:
So my guess is that Cython tries to find a
__cmp__
, and if it doesn't, then (unlike Python) does not use__richcmp__
.
It's not "Cython" which has to implement that: it's Python who decides which comparison function to call. So, it cannot be a bug in Cython.
comment:11 in reply to: ↑ 5 Changed 3 years ago by
Replying to chapoton:
In which file should I put this "custom cmp" function ?
I would put it in sage_object.pxd
where we already have various functions involving comparisons. However, I don't know exactly how we can ensure that this is only used in Python 3.
Maybe a simpler solution would be to patch Cython such that it does recognize cmp()
in Python 3.
comment:12 Changed 3 years ago by
so what is the conclusion for the ticket here ? should I just implement __eq__
?
comment:13 Changed 3 years ago by
Ah, I see my misunderstanding, it is precisely because it is a Python class, not a cdef
Cython class why __richcmp__
is not called. So, I'm thinking that is what needs to happen: we should implement a __eq__
and __ne__
.
comment:14 Changed 3 years ago by
Once I introduced __eq__
and __ne__
, do I need to keep the __richcmp__
or is it completely useless ?
comment:15 Changed 3 years ago by
- Commit changed from b91273eb1952577d423e8b2b689a5b7030b741f0 to ab3a82771ef9b6023c55a79d9e47d5de3cace1dd
comment:16 Changed 3 years ago by
- Status changed from needs_info to needs_review
comment:17 Changed 3 years ago by
The __richcmp__
is something that is only used by cdef
classes, so in this case it is useless.
comment:18 Changed 3 years ago by
- Commit changed from ab3a82771ef9b6023c55a79d9e47d5de3cace1dd to a943b9ab0b07e5d2bbccc9cfea6523c37b143fce
Branch pushed to git repo; I updated commit sha1. New commits:
a943b9a | trac 22048 remove useless__richcmp__
|
comment:19 Changed 3 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
LGTM.
comment:20 Changed 3 years ago by
- Milestone changed from sage-7.5 to sage-7.6
comment:21 Changed 3 years ago by
- Branch changed from u/chapoton/22048 to a943b9ab0b07e5d2bbccc9cfea6523c37b143fce
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
trac #22048 py3 remove one cmp() in pyx file