Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant classcall_private from partitions #14225

Closed
simon-king-jena opened this issue Mar 4, 2013 · 15 comments
Closed

Remove redundant classcall_private from partitions #14225

simon-king-jena opened this issue Mar 4, 2013 · 15 comments

Comments

@simon-king-jena
Copy link
Member

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.

Depends on #13605
Depends on #11410

CC: @sagetrac-sage-combinat @tscrim

Component: combinatorics

Keywords: partition classcall

Author: Simon King

Reviewer: Travis Scrimshaw

Merged: sage-5.8.beta4

Issue created by migration from https://trac.sagemath.org/ticket/14225

@simon-king-jena
Copy link
Member Author

comment:2

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

@simon-king-jena

This comment has been minimized.

@simon-king-jena
Copy link
Member Author

comment:3

I think it is better to keep separate topics on separate tickets. Hence, I only remove __classcall_private__ here, but do not cache stuff.

@tscrim
Copy link
Collaborator

tscrim commented Mar 5, 2013

Changed dependencies from #13605 to #13605 #11410

@tscrim
Copy link
Collaborator

tscrim commented Mar 5, 2013

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Mar 5, 2013

comment:4

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

Thanks Simon,

Travis

@simon-king-jena
Copy link
Member Author

comment:5

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.

@simon-king-jena
Copy link
Member Author

@simon-king-jena
Copy link
Member Author

comment:6

Done.

@tscrim
Copy link
Collaborator

tscrim commented Mar 5, 2013

comment:7

Looks good to me. Thank you,

Travis

@jdemeyer jdemeyer removed this from the sage-5.8 milestone Mar 6, 2013
@simon-king-jena
Copy link
Member Author

comment:9

"Pending" because of #11410?

@jdemeyer
Copy link

jdemeyer commented Mar 6, 2013

comment:10

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

@jdemeyer
Copy link

jdemeyer commented Mar 6, 2013

Changed dependencies from #13605 #11410 to #13605, #11410

@jdemeyer
Copy link

jdemeyer commented Mar 6, 2013

comment:11

Changing my mind because of #14228.

@jdemeyer jdemeyer added this to the sage-5.8 milestone Mar 6, 2013
@jdemeyer jdemeyer removed the pending label Mar 6, 2013
@jdemeyer
Copy link

jdemeyer commented Mar 7, 2013

Merged: sage-5.8.beta4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants