Opened 4 years ago
Closed 4 years ago
#18389 closed defect (fixed)
Fix comparison for MPComplexNumber
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.8 |
Component: | cython | Keywords: | |
Cc: | Merged in: | ||
Authors: | Jeroen Demeyer | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | c448299 (Commits) | Commit: | c448299c76520b9295cf89eef1d614ea482f298e |
Dependencies: | Stopgaps: |
Description
The Cython extension type MPComplexNumber
uses __cmp__
which is not the preferred way to implement comparisons.
Change History (9)
comment:1 Changed 4 years ago by
- Branch set to u/jdemeyer/fix_comparison_for_mpcomplexnumber
comment:2 Changed 4 years ago by
- Commit set to c448299c76520b9295cf89eef1d614ea482f298e
- Status changed from new to needs_review
comment:3 follow-up: ↓ 4 Changed 4 years ago by
Does _cmp_
need to be cpdef
or can it be cdef
(as I thought there was a small speed penalty for the former)? Also can you add a doctest for each of the new (boilerplate) methods?
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 4 years ago by
Replying to tscrim:
Does
_cmp_
need to becpdef
In Sage, there are many Parent
and Element
classes implemented in Python, so it must be cpdef
.
I thought there was a small speed penalty for the former
That's true, but the penalty should be really small.
Also can you add a doctest for each of the new (boilerplate) methods?
There is really no point for that, what could I possibly put in there that is not redundant?
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 4 years ago by
Replying to jdemeyer:
Replying to tscrim:
Does
_cmp_
need to becpdef
In Sage, there are many
Parent
andElement
classes implemented in Python, so it must becpdef
.
Ah, it has to be cpdef
in order to be compatible with the def _cmp_
in Element
.
Also can you add a doctest for each of the new (boilerplate) methods?
There is really no point for that, what could I possibly put in there that is not redundant?
Except for our policy that any new function/method must be documented, but I'm okay with redundant doctests. Perhaps some comparisons with <=
or >=
?
comment:6 in reply to: ↑ 5 Changed 4 years ago by
Replying to tscrim:
but I'm okay with redundant doctests.
I'm not OK with completely pointless and redundant doctests which don't appear in the reference manual anyway.
comment:7 Changed 4 years ago by
Note that the boilerplate comparison methods will be deleted in #18329. So there is even less reason to add doctests since they will be removed in a follow-up ticket.
comment:8 Changed 4 years ago by
- Milestone changed from sage-6.7 to sage-6.8
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Fair enough. Positive review.
comment:9 Changed 4 years ago by
- Branch changed from u/jdemeyer/fix_comparison_for_mpcomplexnumber to c448299c76520b9295cf89eef1d614ea482f298e
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Fix comparison for MPComplexField