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

ddtheta: bin in sep^2 instead of cos(theta) #297

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lgarrison
Copy link
Collaborator

This is a proof-of-concept of binning in squared chord length instead of cos(theta) in DDtheta to help numerical stability as proposed by @JonLoveday in #296. It is far from complete, but seems to give the right answer for the DDtheta fallback kernel. Returning thetaavg is not yet supported, because that needs more thought to make it (1) not terribly slow, and (2) easy enough to port to the SIMD kernels.

For the testing, we should implement a test against a brute-force Python implementation, just like we did for the theory module.

DDsmu uses the dot product to compute cos(theta), but that will have the same issues as the 1-C^2 method because the cosine of theta smaller than ~0.02 degrees will be indistinguishable from 1 in float32 precision. So we ought to do something different there, too.

Help finishing this PR would be greatly appreciated! I probably will not have time to make much progress on it myself.

@lgarrison lgarrison marked this pull request as draft June 24, 2023 19:37
@lgarrison lgarrison changed the title [WIP] ddtheta: bin in sep^2 instead of cos(theta) ddtheta: bin in sep^2 instead of cos(theta) Jun 24, 2023
@manodeep
Copy link
Owner

@lgarrison Thanks for taking the first attempt at fixing the potential catastrophic loss of precision. In terms of balancing the user experience and reasonable expectations vs our development time and resources, this might be better of as a check for the potential loss of precision with float32 (including possibly returning an error) and advising/forcing the user to use float64. This way the user can know/fix the possible error and we can wait until we can implement a uniform treatment of numerical precision throughout the entire code-base.

@lgarrison
Copy link
Collaborator Author

Yes, I think a warning is a reasonable short-term fix. I think 0.2 degrees is 1% error in cos(theta), or 0.5% error in theta; maybe that's a reasonable point at which to emit the warning?

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.

2 participants