Opened 5 years ago

Closed 5 years ago

#22762 closed enhancement (fixed)

py3: get rid of __cmp__ in quadratic forms

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

Status badges

Description

as another micro-step towards python3

Change History (14)

comment:1 Changed 5 years ago by chapoton

  • Branch set to u/chapoton/22762
  • Cc tscrim jmantysalo jdemeyer added
  • Commit set to 4f9194e9e994a3581fb68bf5639e9a90aa180fd9
  • Status changed from new to needs_review

New commits:

9aeefb1posets: order and chain polytopes over ZZ
4f9194epy3: get rid of __cmp__ in quadratic forms

comment:2 Changed 5 years ago by git

  • Commit changed from 4f9194e9e994a3581fb68bf5639e9a90aa180fd9 to 95d4b0ff75985c8c4f7ec7923232fac898301865

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

95d4b0fpy3: get rid of __cmp__ in quadratic forms

comment:3 Changed 5 years ago by tscrim

Do we want to use @total_ordering on the class?

comment:4 Changed 5 years ago by chapoton

What for ? Python3 docs tells it is enough to implement __eq__ and __lt__.

https://docs.python.org/3/library/stdtypes.html#comparisons

comment:5 Changed 5 years ago by tscrim

That is good to know for when we do switch to Python3. Yet, it is not sufficient for Python2 as we need to implement a (trivial) __ne__ for these cases as well.

comment:6 Changed 5 years ago by chapoton

green bot, please review

comment:7 Changed 5 years ago by tscrim

I think we should add the @total_ordering as this change in effect removes functionality while we are using Python2. Otherwise we should go full bore the other direction and remove __lt__.

comment:8 Changed 5 years ago by chapoton

__lt__ is needed because we sometimes sort this kind of things

what is the removed functionality ?

comment:9 Changed 5 years ago by tscrim

For example, with this branch:

sage: P = BinaryQF([2,2,3])
sage: Q = BinaryQF([1,2,3])
sage: Q <= P
False

If this is done for print sorting (the ordering seems artificial to me), then I think those places should have other sorting methods. Is there a specific place where this sorting is used?

comment:10 Changed 5 years ago by chapoton

I tried to remove the __lt__ and got doctest failures in various places. As I did not manage to solve them all, I just drop the problem for the moment.

comment:11 Changed 5 years ago by tscrim

Ah okay. Thanks for trying. I would say we are back to adding @total_ordering to the class; at least this is the simplest approach.

comment:12 Changed 5 years ago by git

  • Commit changed from 95d4b0ff75985c8c4f7ec7923232fac898301865 to e90cb7dcecdab2322a7a01629368fc830285d4f2

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

7d677ffMerge branch 'u/chapoton/22762' in 8.0.b2
e90cb7dtrac 22762 adding total_ordering

comment:13 Changed 5 years ago by tscrim

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

Thanks.

comment:14 Changed 5 years ago by vbraun

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