Opened 7 years ago

Closed 7 years ago

#18322 closed enhancement (fixed)

_cmp should try _richcmp_ if _cmp_ failed

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.7
Component: coercion Keywords:
Cc: vdelecroix Merged in:
Authors: Jeroen Demeyer Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: ac9ca1c (Commits, GitHub, GitLab) Commit: ac9ca1c83737253b380a79cde33b4908a51d0ead
Dependencies: Stopgaps:

Status badges

Description

With the current comparison implementation, defining __cmp__ in a Cython class can actually break cmp() if _cmp_ raises NotImplementedError. This is because defining __cmp__ causes Python to use __cmp__ and only __cmp__. Instead, __cmp__ (so really _cmp in Cython) should itself try _richcmp_ if _cmp_ didn't work.

To avoid an infinite loop, it's important that only _cmp calls _richcmp_, we should not change _cmp_ here.

Change History (14)

comment:1 Changed 7 years ago by jdemeyer

  • Branch set to u/jdemeyer/_cmp_should_try__richcmp__if__cmp__failed

comment:2 Changed 7 years ago by jdemeyer

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

Last 10 new commits:

1ad339bImplement _rich_to_bool as inline function instead of member function
17bd067Merge tag '6.7.beta2' into t/17890/ticket/17890
313a400Optimize rich_to_bool_sgn
629f6a5Improve comparisons for permutation groups
0d1e049Improve _richcmp and documentation
39273f1Fix doctest formatting
04570b3Fix bad doctest in etaproducts
3976f2cAdd pointers for special uses of __richcmp__
1de420cMerge commit '3976f2cc4ec59c9f978d9d02c7f697c81025c4ef' into t/18322/_cmp_should_try__richcmp__if__cmp__failed
c1d1881_cmp should try _richcmp_ if _cmp_ failed

comment:3 Changed 7 years ago by git

  • Commit changed from c1d1881c5034a6712b2efc2c735788494aefbeeb to c70bd3973cc30346d965f3cee6db2648d081be80

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c70bd39_cmp should try _richcmp_ if _cmp_ failed

comment:4 Changed 7 years ago by git

  • Commit changed from c70bd3973cc30346d965f3cee6db2648d081be80 to 621bd1020cadde224d361c0711ddb2ceba2378ed

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

621bd10_cmp should try _richcmp_ if _cmp_ failed

comment:5 Changed 7 years ago by jdemeyer

  • Dependencies #17890 deleted

comment:6 Changed 7 years ago by git

  • Commit changed from 621bd1020cadde224d361c0711ddb2ceba2378ed to cb07254a1a8ec6b862a42492ddfecd0cb238d4e0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cb07254_cmp should try _richcmp_ if _cmp_ failed

comment:7 Changed 7 years ago by git

  • Commit changed from cb07254a1a8ec6b862a42492ddfecd0cb238d4e0 to bb98fd07ee04b18a4ec329aeb2bf061dab4f7d6d

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bb98fd0_cmp should try _richcmp_ if _cmp_ failed

comment:8 follow-up: Changed 7 years ago by vdelecroix

  • Status changed from needs_review to needs_work
  1. The code of _cmp_ raises a NotImplementedError which has a custom error message which involves string formatting. As this error is catched in a critical part of the code (i.e. _cmp) I would either:
    • remove the message "comparison not implemented for %r"%type(left)
    • or use lazy strings

The second option looks better to me.

  1. It would be nice to update this comment:
        ####################################################################
        # For a derived Cython class, you **must** put the __richcmp__
        ...
        # arguments have identical parents.
        ####################################################################
    
    with something like: implementing `_richcmp_` will automatically makes `cmp` works, see the implementation of Element._cmp.
  1. Actually, the current implementation let met think that for elements we should:
    • implement _richcmp_ for more speed (e.g. == is cheap but < is not) or if _cmp_ does not make sense (e.g. we want to test for equality but raise errors on <)
    • implement _cmp_ otherwise

Am I right?

Vincent

comment:9 Changed 7 years ago by git

  • Commit changed from bb98fd07ee04b18a4ec329aeb2bf061dab4f7d6d to d826520e60f02169ff9b93e2e6c29eb662d1d14f

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

d826520Use LazyFormat for _cmp_ exception

comment:10 in reply to: ↑ 8 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

  1. Actually, the current implementation let met think that for elements we should:
    • implement _richcmp_ for more speed (e.g. == is cheap but < is not) or if _cmp_ does not make sense (e.g. we want to test for equality but raise errors on <)
    • implement _cmp_ otherwise

I think so, yes.

comment:11 Changed 7 years ago by git

  • Commit changed from d826520e60f02169ff9b93e2e6c29eb662d1d14f to ac9ca1c83737253b380a79cde33b4908a51d0ead

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

ac9ca1cUpdate comment

comment:12 Changed 7 years ago by jdemeyer

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_work to needs_review

comment:13 Changed 7 years ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:14 Changed 7 years ago by vbraun

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