Opened 21 months ago
Closed 21 months ago
#26181 closed enhancement (fixed)
py3: cleanup of the Cusps class
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.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 )
also part of #24551
Change History (16)
comment:1 Changed 21 months ago by
 Branch set to u/chapoton/26181
 Status changed from new to needs_review
comment:2 Changed 21 months ago by
 Commit set to 331c40823b4a3cec4b58c4bcaaff62f9af4b071b
comment:3 Changed 21 months ago by
 Description modified (diff)
comment:4 Changed 21 months ago by
What about making Cusps_class
a subclass of sage.misc.fast_methods.Singleton
?
comment:5 Changed 21 months ago by
What would be the benefit ?
EDIT: should I just replace UniqueRepresentation
by Singleton
?
comment:6 Changed 21 months ago by
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 followup: ↓ 8 Changed 21 months ago by
Replacing UniqueRepresentation by Singleton does not work (conflict with Parent ?)
comment:8 in reply to: ↑ 7 Changed 21 months ago by
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 21 months ago by
 Commit changed from 331c40823b4a3cec4b58c4bcaaff62f9af4b071b to c66bc06ac74dc86c8d0e62d0b39a45e549c928bf
Branch pushed to git repo; I updated commit sha1. New commits:
c66bc06  using Singleton

comment:10 Changed 21 months ago by
ok, thanks. I did put it in second position.
comment:11 Changed 21 months ago by
 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:13 Changed 21 months ago by
 Commit changed from c66bc06ac74dc86c8d0e62d0b39a45e549c928bf to 4d4914002cf3409cac8ab535f2b85ef5dd71559c
Branch pushed to git repo; I updated commit sha1. New commits:
4d49140  trac 26181 changing doctests in parent_old.pyx

comment:14 Changed 21 months ago by
 Status changed from needs_work to needs_review
should be better now..
comment:15 Changed 21 months ago by
 Status changed from needs_review to positive_review
LGTM. Thanks.
comment:16 Changed 21 months ago by
 Branch changed from u/chapoton/26181 to 4d4914002cf3409cac8ab535f2b85ef5dd71559c
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
py3: cleanup of the Cusps class