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

new acb_theta module #1470

Merged
merged 349 commits into from
Nov 22, 2023
Merged

new acb_theta module #1470

merged 349 commits into from
Nov 22, 2023

Conversation

edgarcosta
Copy link
Member

@edgarcosta edgarcosta commented Oct 9, 2023

The purpose of this module is to provide methods for the numerical evaluation of theta functions in any dimension.

I make this PR as a draft, as I am not sure if @j-kieffer thinks it is ready for review.
But I think it would be helpful to start looking at it, particularly the auxiliary code that does the obvious things.

Here is some documentation documentation

ps: @j-kieffer, no pressure in working on the code this week, just there are a couple of folks here this week who are keen on looking into it.

@j-kieffer
Copy link
Contributor

Another thing we talked about is having documentation for the tests. It wouldn't be too much for me to write in in the rst file (or somewhere else), but it would clash with the rest of FLINT.

@fredrik-johansson
Copy link
Collaborator

Another thing we talked about is having documentation for the tests. It wouldn't be too much for me to write in in the rst file (or somewhere else), but it would clash with the rest of FLINT.

Sets a precedent to improve the rest of FLINT then ;-)

@j-kieffer
Copy link
Contributor

Ok! Do you have a preference between documenting each test right after the function as I did in the pdf documentation before, or at the end of the rst file?

@albinahlback
Copy link
Collaborator

albinahlback commented Nov 13, 2023

Ok! Do you have a preference between documenting each test right after the function as I did in the pdf documentation before, or at the end of the rst file?

I would prefer to have them at the end of the file, so that it preserves the readability for the user part of the documentation. Very nice idea to document the tests!

@j-kieffer
Copy link
Contributor

j-kieffer commented Nov 15, 2023

I wrote documentation for the tests (and profiling code while I was at it) in 71d2a46. If you have any thoughts on the format or wording let me know!

@fredrik-johansson
Copy link
Collaborator

The documentation looks excellent!

I tried to compare the performance of acb_modular_theta and acb_theta_all for tau = i, z = sqrt(2) and I get a crossover around 2-3 million bits. Is that reasonable? For z = 0 the crossover looks astronomical.

It's worth noting that acb_modular_theta has some code for detecting special cases where the real or imaginary part in the output is exactly zero, so one gets a prettier (and slightly more precise) enclosure. Is that something possible and worth doing for g > 1 also?

@fredrik-johansson
Copy link
Collaborator

For theta constants with tau = i, there is still a 4x difference between acb_modular_theta and acb_theta_all at 10^8 bits, which is around where acb_modular_theta runs out of memory on my machine. Obviously, losing a factor 4x in speed at that point is better than killing the process...

@edgarcosta
Copy link
Member Author

I would say unless those changes are trivial, we should postpone implementing those optimizations.
If you go that path, we would for sure appreciate some breadcrumbs on how to achieve those.

@fredrik-johansson
Copy link
Collaborator

Yeah, crossovers between the classical and asymptotically fast implementations in the g = 1 case are supposed to go in a future PR. Just asking if I'm seeing the expected results.

@j-kieffer
Copy link
Contributor

No, somehow that's not the results I would expect - I would expect the crossover to be much earlier, at a few thousand bits maybe. (I remember observing this before.) I'll try to investigate this but I'll be busy with other things until Monday. In the meantime, can you try to do timings for acb_theta_ql_all as well? Compared to acb_theta_all, this only does the duplication formula, not the reduction step + action of the modular group.

@j-kieffer
Copy link
Contributor

j-kieffer commented Nov 16, 2023

For the other question about special values, yes, we still have such properties in higher dimensions - e.g. theta constants are real when the matrix is purely imaginary, I believe. I haven't implemented this yet, but it wouldn't be difficult.

@fredrik-johansson
Copy link
Collaborator

In any case, don't hesitate to do a final rebase and unset the draft status on this PR. The current code is excellent and I'd be happy to merge it sooner rather than later.

@fredrik-johansson
Copy link
Collaborator

There's basically no difference between acb_theta_ql_all and acb_theta_all. The latter is a little faster.

@j-kieffer
Copy link
Contributor

Thanks, I'll take a look then! Github tells me "Only those with write access to this repository can mark a draft pull request as ready for review."

@fredrik-johansson fredrik-johansson marked this pull request as ready for review November 18, 2023 13:55
@fredrik-johansson
Copy link
Collaborator

OK, fixed.

@j-kieffer
Copy link
Contributor

The crossover between acb_theta_all and acb_modular_theta should be around 6000 bits now (and roughly 50000 for theta constants). I'll let the checks complete and run valgrind one last time, otherwise I approve the merge!

@j-kieffer
Copy link
Contributor

Valgrind passed.

@fredrik-johansson
Copy link
Collaborator

The crossover between acb_theta_all and acb_modular_theta should be around 6000 bits now (and roughly 50000 for theta constants). I'll let the checks complete and run valgrind one last time, otherwise I approve the merge!

Confirmed!

@fredrik-johansson fredrik-johansson merged commit 0de2b27 into flintlib:trunk Nov 22, 2023
14 checks passed
@edgarcosta
Copy link
Member Author

Thank you all!

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

Successfully merging this pull request may close these issues.

6 participants