Opened 10 years ago

Closed 10 years ago

#14225 closed enhancement (fixed)

Remove redundant classcall_private from partitions

Reported by: Simon King Owned by: Sage Combinat CC user
Priority: major Milestone: sage-5.8
Component: combinatorics Keywords: partition classcall
Cc: Sage Combinat CC user, Travis Scrimshaw Merged in: sage-5.8.beta4
Authors: Simon King Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13605, #11410 Stopgaps:

Status badges

Description (last modified by Simon King)

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)

trac_14225-partition_classcall_private.patch (9.6 KB) - added by Simon King 10 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by Simon King

Status: newneeds_review

comment:2 Changed 10 years ago by Simon King

Oops, I forgot to document the new cached function. Patch updated.

comment:3 Changed 10 years ago by Simon King

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 10 years ago by Travis Scrimshaw

Dependencies: #13605#13605 #11410
Reviewers: Travis Scrimshaw

Could you rebase this over #11410 (which is essentially done)? Other than that, it looks good.

Thanks Simon,
Travis

comment:5 Changed 10 years ago by Simon King

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 10 years ago by Simon King

comment:6 Changed 10 years ago by Simon King

Done.

comment:7 Changed 10 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

Looks good to me. Thank you,
Travis

comment:8 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.8sage-pending

comment:9 Changed 10 years ago by Simon King

"Pending" because of #11410?

comment:10 Changed 10 years ago by Jeroen Demeyer

Yes, but in any case it would be sage-5.9, not sage-5.8.

comment:11 Changed 10 years ago by Jeroen Demeyer

Dependencies: #13605 #11410#13605, #11410
Milestone: sage-pendingsage-5.8

Changing my mind because of #14228.

comment:12 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.8.beta4
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.