Opened 2 years ago

Closed 2 years ago

#25419 closed enhancement (fixed)

py3: remove one __cmp__ in linear functions

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.3
Component: python3 Keywords: python3, richcmp
Cc: embray, jdemeyer, tscrim, fbissey Merged in:
Authors: Frédéric Chapoton Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 820f618 (Commits) Commit: 820f61840f783dec76894396b8b79a6dbb49fbab
Dependencies: Stopgaps:

Description

part of #16537

Change History (19)

comment:1 Changed 2 years ago by chapoton

  • Branch set to public/25419
  • Commit set to f6dfe5bb66deb078ed27f6ba6864934110a3592d
  • Status changed from new to needs_review

New commits:

f6dfe5bpy3: remove one __cmp__

comment:2 Changed 2 years ago by chapoton

  • Keywords python3 richcmp added

comment:3 Changed 2 years ago by chapoton

bot is morally green. The timeout doctest failure in padics is seen on every ticket with this patchbot.

comment:4 Changed 2 years ago by chapoton

  • Cc embray jdemeyer tscrim fbissey added

comment:5 Changed 2 years ago by jdemeyer

I'm a bit worried because this will break some of the doctests

            sage: len(set([f, f]))
            1
            sage: len(set([f, f+0]))
            2
            sage: len(set([f, f+1]))
            2

so it's not just a harmless change, it actually changes functionality.

To be honest, I don't know how to fix that. So maybe we should just live with it. In that case, we should at least document it though.

comment:6 Changed 2 years ago by chapoton

Not on holidays, Jeroen ? :)

Well, the way this class uses richcmp makes comparison fundamentally broken.

If you have any idea on how best to deal with that, please take over.

comment:7 Changed 2 years ago by embray

This change doesn't actually appear to break any of those tests. If nothing else you might keep the tests in a TESTS: block somewhere in the class. But otherwise this __cmp__ appears to be superfluous.

comment:8 Changed 2 years ago by git

  • Commit changed from f6dfe5bb66deb078ed27f6ba6864934110a3592d to 4b7f3a0773f5dc838441f99ba7e6a36e7bef3ad1

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

4b7f3a0adding back some doctests

comment:9 Changed 2 years ago by chapoton

I have added back these doctests.

comment:10 Changed 2 years ago by chapoton

ping ?

comment:11 Changed 2 years ago by embray

There's some patchbot failures, but, it looks more like broken patchbots...

comment:12 Changed 2 years ago by chapoton

ping ? (green bots on 8.3.b4)

comment:13 Changed 2 years ago by chapoton

Jeroen, your opinion please ?

comment:14 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

The reason that the set() tests still pass is because hashing is by id. So that's actually a good thing because the bogus __richcmp__ is then not an obstruction for using linear functions as dict key.

So I agree with this ticket, provided that some docs and comments are updated. I'll do that myself.

comment:15 Changed 2 years ago by git

  • Commit changed from 4b7f3a0773f5dc838441f99ba7e6a36e7bef3ad1 to 820f61840f783dec76894396b8b79a6dbb49fbab

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

4913e13py3: remove one __cmp__
b679726adding back some doctests
820f618Clarify __hash__

comment:16 Changed 2 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_work to needs_review

Last commit needs review.

comment:17 Changed 2 years ago by chapoton

ok, good for me. Let us wait for the patchbot approval

comment:18 Changed 2 years ago by chapoton

  • Status changed from needs_review to positive_review

comment:19 Changed 2 years ago by vbraun

  • Branch changed from public/25419 to 820f61840f783dec76894396b8b79a6dbb49fbab
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.