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 jdemeyer

  • Branch set to u/jdemeyer/fix_comparison_for_mpcomplexnumber

comment:2 Changed 4 years ago by jdemeyer

  • Commit set to c448299c76520b9295cf89eef1d614ea482f298e
  • Status changed from new to needs_review

New commits:

c448299Fix comparison for MPComplexField

comment:3 follow-up: Changed 4 years ago by tscrim

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: Changed 4 years ago by jdemeyer

Replying to tscrim:

Does _cmp_ need to be cpdef

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: Changed 4 years ago by tscrim

Replying to jdemeyer:

Replying to tscrim:

Does _cmp_ need to be cpdef

In Sage, there are many Parent and Element classes implemented in Python, so it must be cpdef.

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 jdemeyer

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 jdemeyer

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 tscrim

  • 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 vbraun

  • Branch changed from u/jdemeyer/fix_comparison_for_mpcomplexnumber to c448299c76520b9295cf89eef1d614ea482f298e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.