#13199 closed enhancement (fixed)
Use FLINT to compute the partition function
Reported by: | fredrik.johansson | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-5.11 |
Component: | combinatorics | Keywords: | |
Cc: | Merged in: | sage-5.11.beta1 | |
Authors: | Fredrik Johansson | Reviewers: | Andrew Mathas, Frédéric Chapoton, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Before:
sage: %timeit number_of_partitions(1) 625 loops, best of 3: 2.09 µs per loop sage: %timeit number_of_partitions(10^3) 625 loops, best of 3: 197 µs per loop sage: %timeit number_of_partitions(10^6) 25 loops, best of 3: 32.8 ms per loop sage: %time number_of_partitions(10^9); CPU times: user 37.92 s, sys: 0.00 s, total: 37.92 s Wall time: 37.93 s
After:
sage: %timeit number_of_partitions(1) 625 loops, best of 3: 1.94 µs per loop sage: %timeit number_of_partitions(10^3) 625 loops, best of 3: 51.4 µs per loop sage: %timeit number_of_partitions(10^6) 125 loops, best of 3: 2.66 ms per loop sage: %time number_of_partitions(10^9); CPU times: user 0.47 s, sys: 0.00 s, total: 0.47 s Wall time: 0.48 s
TODO: should add a 64-bit only test to check that evaluation works with n >= 2^{32. }
Apply:
Attachments (3)
Change History (23)
Changed 8 years ago by
comment:1 Changed 8 years ago by
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
- Reviewers set to Andrew Mathas
comment:3 Changed 8 years ago by
- Status changed from needs_review to needs_work
comment:4 Changed 7 years ago by
- Dependencies #12173 deleted
I removed the dependency to #12173, since this has been closed in 5.10.beta3, and since a dependency on spkg makes the bot unhappy.
for the bot:
apply flint_partition_function.patch
comment:5 Changed 7 years ago by
- Work issues set to needs rebase
this needs to be rebased on 5.10beta4
Changed 7 years ago by
comment:6 Changed 7 years ago by
for the bot:
apply trac_13199_flint_partition_function_v2.patch
I have made a rebased patch, passing all tests.
comment:7 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:8 Changed 7 years ago by
Hi. As I said on the ticket above, can you please add some more tests to the new partition function.
comment:9 Changed 7 years ago by
ok, I will add the tests. But there is an annoying doctest failure concerning cached functions, see bot report.
comment:10 Changed 7 years ago by
- Description modified (diff)
- Reviewers changed from Andrew Mathas to Andrew Mathas, Frederic Chapoton, Travis Scrimshaw
Here's a review patch which should fix the problem, which was the Bober's implementation was cached (since it was the default). I changed this to the FLINT.
sage -t ../misc/cachefunc.pyx [593 tests, 78.50 s] ---------------------------------------------------------------------- All tests passed! ---------------------------------------------------------------------- Total time for all tests: 79.6 seconds cpu time: 8.1 seconds cumulative wall time: 78.5 seconds
I also added the additional tests Andrew wanted and fixed the docstrings.
For patchbot:
Apply: trac_13199_flint_partition_function_v2.patch, trac_13199-flint_partition-review-ts.patch
comment:11 Changed 7 years ago by
Looks good to me, but the bot complains about the new module. Is there a way to avoid that, or should we just forget this warning ?
comment:12 Changed 7 years ago by
The new module can't really be avoided, plus it doesn't (significantly) add to the startup time, so I ignore these.
comment:13 Changed 7 years ago by
- Status changed from needs_review to positive_review
I'm going to set this to positive review now since I don't think either of you (Andrew, Frederic) have any other objections. Feel free to set this to needs work if I'm wrong.
comment:14 Changed 7 years ago by
- Milestone changed from sage-5.10 to sage-5.11
- Work issues needs rebase deleted
Changed 7 years ago by
comment:15 Changed 7 years ago by
- Status changed from positive_review to needs_work
I've uploaded a new version of my review patch which removes the unneeded include_dirs
from module_list.py
.
comment:16 Changed 7 years ago by
- Status changed from needs_work to needs_review
Because of the type of change, this needs another (quick) review.
comment:17 follow-up: ↓ 18 Changed 7 years ago by
- Status changed from needs_review to positive_review
ok, looks good to me.
comment:18 in reply to: ↑ 17 Changed 7 years ago by
comment:19 Changed 7 years ago by
- Merged in set to sage-5.11.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
comment:20 Changed 7 years ago by
- Reviewers changed from Andrew Mathas, Frederic Chapoton, Travis Scrimshaw to Andrew Mathas, Frédéric Chapoton, Travis Scrimshaw
I am happy to review this, although it is not clear to me what the status of #12173 is and until this patch is merged it seems rather cumbersome to load the current patch on top of #12173 in order play around with this patch and check to see that it actually works.
Assuming that it is OK, I would like to see some more explicit tests inside the new number_of_partitions to show that it is working. I'd suggest simply reusing the ones that are in sage.combinat.partition.number_of_partitions. That is, adding the following tests to the doc-string for your number_of_partitions: