Opened 2 years ago

Closed 2 years ago

#26181 closed enhancement (fixed)

py3: cleanup of the Cusps class

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.4
Component: python3 Keywords:
Cc: Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 4d49140 (Commits) Commit: 4d4914002cf3409cac8ab535f2b85ef5dd71559c
Dependencies: Stopgaps:

Description (last modified by chapoton)

also part of #24551

Change History (16)

comment:1 Changed 2 years ago by chapoton

  • Branch set to u/chapoton/26181
  • Status changed from new to needs_review

comment:2 Changed 2 years ago by git

  • Commit set to 331c40823b4a3cec4b58c4bcaaff62f9af4b071b

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

​331c408py3: cleanup of the Cusps class

comment:3 Changed 2 years ago by chapoton

  • Description modified (diff)

comment:4 Changed 2 years ago by tscrim

What about making Cusps_class a subclass of sage.misc.fast_methods.Singleton?

comment:5 Changed 2 years ago by chapoton

What would be the benefit ?

EDIT: should I just replace UniqueRepresentation by Singleton ?

Last edited 2 years ago by chapoton (previous) (diff)

comment:6 Changed 2 years ago by tscrim

Singleton is faster (which probably means we can remove Cusps nailed in memory) and IMO makes the design more clear as it conveys some more semantics.

comment:7 follow-up: Changed 2 years ago by chapoton

Replacing UniqueRepresentation by Singleton does not work (conflict with Parent ?)

comment:8 in reply to: ↑ 7 Changed 2 years ago by tscrim

Replying to chapoton:

Replacing UniqueRepresentation by Singleton does not work (conflict with Parent ?)

What goes wrong? It shouldn't conflict with Parent, but it does have to come first because it needs to be at the bottom of the MRO (so there also cannot be any subclasses).

comment:9 Changed 2 years ago by git

  • Commit changed from 331c40823b4a3cec4b58c4bcaaff62f9af4b071b to c66bc06ac74dc86c8d0e62d0b39a45e549c928bf

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

​c66bc06using Singleton

comment:10 Changed 2 years ago by chapoton

ok, thanks. I did put it in second position.

comment:11 Changed 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

Thanks.

It probably should also use the Cusps.element_class instead of Cusp, but there is not any benefit right now (as nothing is really gained from the category). So that can wait for a followup if you want to just set a positive review now.

comment:12 Changed 2 years ago by chapoton

  • Status changed from needs_review to needs_work

failing doctests

comment:13 Changed 2 years ago by git

  • Commit changed from c66bc06ac74dc86c8d0e62d0b39a45e549c928bf to 4d4914002cf3409cac8ab535f2b85ef5dd71559c

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

​4d49140trac 26181 changing doctests in parent_old.pyx

comment:14 Changed 2 years ago by chapoton

  • Status changed from needs_work to needs_review

should be better now..

comment:15 Changed 2 years ago by tscrim

  • Status changed from needs_review to positive_review

LGTM. Thanks.

comment:16 Changed 2 years ago by vbraun

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