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 SimonKing)

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 SimonKing 8 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by SimonKing

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by SimonKing

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

comment:3 Changed 8 years ago by SimonKing

  • 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 tscrim

  • 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 SimonKing

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 SimonKing

comment:6 Changed 8 years ago by SimonKing

Done.

comment:7 Changed 8 years ago by tscrim

  • Status changed from needs_review to positive_review

Looks good to me. Thank you,
Travis

comment:8 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.8 to sage-pending

comment:9 Changed 8 years ago by SimonKing

"Pending" because of #11410?

comment:10 Changed 8 years ago by jdemeyer

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

comment:11 Changed 8 years ago by jdemeyer

  • 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 jdemeyer

  • Merged in set to sage-5.8.beta4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.