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
Use FLINT to compute the partition function #13199
Comments
Attachment: flint_partition_function.patch.gz |
comment:2
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:
|
Reviewer: Andrew Mathas |
Changed dependencies from #12173 to none |
comment:4
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 |
Work Issues: needs rebase |
comment:5
this needs to be rebased on 5.10beta4 |
comment:6
Attachment: trac_13199_flint_partition_function_v2.patch.gz for the bot: apply trac_13199_flint_partition_function_v2.patch I have made a rebased patch, passing all tests. |
comment:8
Hi. As I said on the ticket above, can you please add some more tests to the new partition function. |
comment:9
ok, I will add the tests. But there is an annoying doctest failure concerning cached functions, see bot report. |
This comment has been minimized.
This comment has been minimized.
Changed reviewer from Andrew Mathas to Andrew Mathas, Frederic Chapoton, Travis Scrimshaw |
comment:10
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.
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
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
The new module can't really be avoided, plus it doesn't (significantly) add to the startup time, so I ignore these. |
comment:13
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. |
Changed work issues from needs rebase to none |
Attachment: trac_13199-flint_partition-review-ts.patch.gz |
comment:15
I've uploaded a new version of my review patch which removes the unneeded |
comment:16
Because of the type of change, this needs another (quick) review. |
comment:17
ok, looks good to me. |
comment:18
Replying to @fchapoton:
Thanks Travis. I'm happy too. |
Merged: sage-5.11.beta1 |
Changed reviewer from Andrew Mathas, Frederic Chapoton, Travis Scrimshaw to Andrew Mathas, Frédéric Chapoton, Travis Scrimshaw |
Before:
After:
TODO: should add a 64-bit only test to check that evaluation works with n >= 2^32.
Apply:
Component: combinatorics
Author: Fredrik Johansson
Reviewer: Andrew Mathas, Frédéric Chapoton, Travis Scrimshaw
Merged: sage-5.11.beta1
Issue created by migration from https://trac.sagemath.org/ticket/13199
The text was updated successfully, but these errors were encountered: