Opened 8 years ago
Closed 8 years ago
#14225 closed enhancement (fixed)
Remove redundant classcall_private from partitions
Reported by: | SimonKing | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-5.8 |
Component: | combinatorics | Keywords: | partition classcall |
Cc: | sage-combinat, tscrim | Merged in: | sage-5.8.beta4 |
Authors: | Simon King | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #13605, #11410 | Stopgaps: |
Description (last modified by )
This is a follow-up to #13605. I think that several __classcall_private__
methods in sage.combinat.partition are not needed.
There are some __classcall_private__
methods that do an extensive preprocessing, or that deal with optional arguments. I work on making the latter automatic. But currently, thee __classcall_private__
applications make sense.
However, I doubt that a __classcall_private__
method makes sense that simply puts a Integer(k)
around an argument k
that is supposed to be an integer (python int or whatever). Since caching is by equality and since k as int, Integer, NN, QQ etc. evaluates equal, caching is not an issue here.
Attachments (1)
Change History (13)
comment:1 Changed 8 years ago by
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
comment:3 Changed 8 years ago by
- Description modified (diff)
I think it is better to keep separate topics on separate tickets. Hence, I only remove __classcall_private__
here, but do not cache stuff.
comment:4 Changed 8 years ago by
- Dependencies changed from #13605 to #13605 #11410
- Reviewers set to Travis Scrimshaw
Could you rebase this over #11410 (which is essentially done)? Other than that, it looks good.
Thanks Simon,
Travis
comment:5 Changed 8 years ago by
I am just thinking: Shouldn't Partitions()
(called without arguments) be cached? This is only a single value, and it occurs frequently in the code.
Changed 8 years ago by
comment:6 Changed 8 years ago by
Done.
comment:7 Changed 8 years ago by
- Status changed from needs_review to positive_review
Looks good to me. Thank you,
Travis
comment:8 Changed 8 years ago by
- Milestone changed from sage-5.8 to sage-pending
comment:9 Changed 8 years ago by
"Pending" because of #11410?
comment:10 Changed 8 years ago by
Yes, but in any case it would be sage-5.9, not sage-5.8.
comment:11 Changed 8 years ago by
- Dependencies changed from #13605 #11410 to #13605, #11410
- Milestone changed from sage-pending to sage-5.8
Changing my mind because of #14228.
comment:12 Changed 8 years ago by
- Merged in set to sage-5.8.beta4
- Resolution set to fixed
- Status changed from positive_review to closed
Oops, I forgot to document the new cached function. Patch updated.