search

manodeep / Corrfunc

⚡️⚡️⚡️Blazing fast correlation functions on the CPU.
https://corrfunc.readthedocs.io
MIT License
163 stars 50 forks source link

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

Open lgarrison opened 1 year ago

lgarrison commented 1 year ago

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.

manodeep commented 1 year ago

@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 commented 1 year ago

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?