Opened 4 years ago

Closed 4 years ago

#25053 closed enhancement (fixed)

py3: remove __cmp__ in free monoids

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords: python3, richcmp
Cc: embray, jdemeyer, fbissey Merged in:
Authors: Frédéric Chapoton Reviewers: Jeroen Demeyer, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 26de175 (Commits, GitHub, GitLab) Commit: 26de175f2507e38a68e127d852bfad1ebfecb076
Dependencies: Stopgaps:

Status badges

Description

part of #16537

Change History (15)

comment:1 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/23053
  • Commit set to 2d164093043f15e86412bf3d9d9bea0a709ebc5e
  • Status changed from new to needs_review

New commits:

2d16409remove __cmp__ in free monoids

comment:2 Changed 4 years ago by chapoton

  • Status changed from needs_review to needs_work

many failing doctests

comment:3 Changed 4 years ago by git

  • Commit changed from 2d164093043f15e86412bf3d9d9bea0a709ebc5e to f8d87288fb6d681a57750d188c093b5a578d4143

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

2768646Merge branch 'u/chapoton/23053' in 8.2.rc0
f8d8728better richcmp for free monoid

comment:4 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

should be better now

comment:5 Changed 4 years ago by chapoton

  • Cc embray jdemeyer fbissey added

bot is green, please review

comment:6 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Can this actually happen?

        if not isinstance(right, FreeMonoidElement)

Certainly, this cannot happen in _richcmp_:

             left.parent() != right.parent()):

comment:7 Changed 4 years ago by jdemeyer

Also: please use self, other instead of left, right.

comment:8 Changed 4 years ago by git

  • Commit changed from f8d87288fb6d681a57750d188c093b5a578d4143 to 9405cde60dc9d6b00cdb608eb5334d8d12786939

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

9405cdemore details for _richcmp_ in free monoids

comment:9 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

Thanks, done.

comment:10 Changed 4 years ago by chapoton

bot is green again

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

Minor point, but it is quite a bit faster to do tuple([gen]) than tuple(gen):

sage: %timeit tuple([x for x in range(1000)])
10000 loops, best of 3: 28.1 µs per loop
sage: %timeit tuple(x for x in range(1000))
10000 loops, best of 3: 40.6 µs per loop

comment:12 Changed 4 years ago by git

  • Commit changed from 9405cde60dc9d6b00cdb608eb5334d8d12786939 to 26de175f2507e38a68e127d852bfad1ebfecb076

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

26de175trac 25053 some details

comment:13 Changed 4 years ago by tscrim

  • Reviewers set to Jeroen Demeyer, Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:14 in reply to: ↑ 11 Changed 4 years ago by embray

Replying to tscrim:

Minor point, but it is quite a bit faster to do tuple([gen]) than tuple(gen):

Weird, why do you think that is? And does that hold for even larger generators? Seems like it's just unnecessary duplication to me.

comment:15 Changed 4 years ago by vbraun

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