Opened 4 years ago

Closed 4 years ago

#21980 closed enhancement (fixed)

py3 richcmp in real_double pyx

Reported by: chapoton Owned by:
Priority: major Milestone: sage-7.5
Component: python3 Keywords:
Cc: tscrim, aapitzsch, jdemeyer Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: dbeac6d (Commits, GitHub, GitLab) Commit: dbeac6d608d4d26db0404ccb1bd9a6a569c2ab17
Dependencies: Stopgaps:

Status badges

Description

a minuscule step to python3

Change History (15)

comment:1 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/21980
  • Commit set to 5eb675f404462f5af4225f223838638e2c1595e7

New commits:

​5eb675fpy3 richcmp in real_double.pyx

comment:2 Changed 4 years ago by jdemeyer

_richcmp_ only makes sense for Element classes. Do we have a general strategy for Parent classes?

comment:3 Changed 4 years ago by chapoton

I would say parents should only compare equal if they are the same (in which sense ?).

Converting to __richcmp__ seems to be causing problems with pickling.

I have currently no idea what to do, but this is standing on our long way to py3.

comment:4 Changed 4 years ago by tscrim

Perhaps change it to an __eq__ and add a __ne__?

comment:5 Changed 4 years ago by tscrim

Actually, I think what was causing the problems with simply changing to __richcmp__ is with the blanket return NotImplemented. This was not actually checking !=.

comment:6 Changed 4 years ago by git

  • Commit changed from 5eb675f404462f5af4225f223838638e2c1595e7 to 4d96d1c560cbc6e3464b9a053e874f4c84fd3500

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

​b69406fMerge branch 'u/chapoton/21980' in 7.5.b5
​4d96d1ctrac 21980 __richcmp__ may work

comment:7 Changed 4 years ago by chapoton

  • Cc tscrim aapitzsch jdemeyer added
  • Status changed from new to needs_review

this seems to be good to go, the latex problem is most probably unrelated

comment:8 Changed 4 years ago by chapoton

*ping*

comment:9 Changed 4 years ago by tscrim

My opinion is it should return True for != if the classes are distinct, which is a sufficient test for !=.

comment:10 Changed 4 years ago by jdemeyer

Like I said, I think we should have a general plan for comparing parents. We do have one for elements (the coercion model). As long as we don't have such a plan, I rather wouldn't want to change __cmp__ to __richcmp__.

comment:11 follow-up: Changed 4 years ago by chapoton

In Parent, we only have _cmp_ which calls __cmp__ and _richcmp which call _cmp_

I think one should add there a _richcmp_ (possibly replacing the existing _richcmp) that first tries to call __richcmp__ and then falls back to calling __cmp__.

Should this be looked at in #22029 or in a new ticket ?

comment:12 in reply to: ↑ 11 Changed 4 years ago by jdemeyer

Replying to chapoton:

Should this be looked at in #22029 or in a new ticket ?

Certainly a new ticket.

comment:13 Changed 4 years ago by git

  • Commit changed from 4d96d1c560cbc6e3464b9a053e874f4c84fd3500 to dbeac6d608d4d26db0404ccb1bd9a6a569c2ab17

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

​6f39979Merge branch 'u/chapoton/21980' in 7.5.b6
​dbeac6dtrac 21980 True for NE

comment:14 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

I think this will do for now, but the reason we may not see any pickling issues here is because this has a (unsophisticated) way of implementing singleton behavior. So if we change this to an instance of Singleton, then we don't need the __richcmp__, but that is for another ticket.

comment:15 Changed 4 years ago by vbraun

  • Branch changed from u/chapoton/21980 to dbeac6d608d4d26db0404ccb1bd9a6a569c2ab17
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.