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

Compute bin_uiui via Flint for small n #383

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

albinahlback
Copy link
Contributor

Big speedup for smaller n and increased precision due to not using floating point precision.

Let me know if you want any timings for this.

@albinahlback
Copy link
Contributor Author

Sorry. Didn't include flint/fmpz.h the first time and didn't include the prefix flint/ the second time.

@albinahlback
Copy link
Contributor Author

Does it fail because MPIR might be used?

@albinahlback
Copy link
Contributor Author

Does this look good @fredrik-johansson ?

@fredrik-johansson
Copy link
Collaborator

It's ok, but I'd like to see cutoffs based on n, k and the precision.

Computing bin(n, k) via GMP should be faster even for huge n, if k is small or if log2(bin(n, k)) is less than some small multiple of prec.

It would be nice if also arb_bin_ui called the same algorithm when n is a small integer.

@albinahlback
Copy link
Contributor Author

Yes, that's true! Thanks for your input.

@albinahlback
Copy link
Contributor Author

I haven't tested the code yet, but I think the thoughts are solid.

I profiled with 10 bits in order to give the original code an advantage, and found that the bounds in the code are pretty good estimates for when the new version is faster. I had to do some extra code in case that ulong != unsigned long and in case that FLINT_BITS != 64.


#define LOG2_BIN(n, k) \
(-1.3257480647361592 \
+ ((n) + .5) * log2(n) \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think log2 might not be available everywhere. Better to use log.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"C99"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your opinion about supporting/assuming newer standards?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be needed at some point, but not worth risking any breakage right here.

else
{
arf_struct atmp;
mag_struct mtmp = {0, 0};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This level of code shouldn't be be written in a way that relies on the internal representation of structs.

else if (k < (UWORD(1) << 32) && ((double) prec) < LOG2_BIN(n, k))
{
mpz_t mres;
__mpz_struct mn = {1, 1, NULL};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This level of code shouldn't be be written in a way that relies on the internal representation of structs.

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

Successfully merging this pull request may close these issues.

2 participants