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

changing normalization to standard #146

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

Conversation

boryanah
Copy link
Collaborator

@boryanah boryanah commented Aug 5, 2024

It seems like the only place that because the only case in which the normalization matters is when the number of pcles is different from the number of grid points, the ZCV method is safe (tested Pk but not Xi), so the normalization can be fixed with a single line change.

I will also try to add to this the correction to lc env calculation for AbacusHOD

@lgarrison
Copy link
Member

Looks good! Any reason to comment out the old code rather than just remove it?

Also, since this change affects ZCV, this might be a good time to think about adding tests to ZCV. Right now, I think we just check that it runs, but not that the result is correct.

@boryanah
Copy link
Collaborator Author

boryanah commented Aug 6, 2024

Hm, that sounds good -- I think it is not 100% straightforward how to implement a small test as the operations are somewhat expensive; perhaps a very simple test could be just comparing the outputs (as nonsensical as they are) for this mini simulation -- my worry is that it might fail the test if we improve the code in any major ways, but then we would just change the reference so that is fine. This proposed test would perhaps have the benefit of making sure that the output is unchanged when e.g. we change things like units, formats, etc. let me know what you think.

Also let me know if you think the environment computation change makes sense.

@lgarrison
Copy link
Member

Yeah, testing that things are unchanged (rather than checking against a known-correct result) is probably the best we can do, but I do think it's good given that optimization of this code is important, and we need to know that when we optimize we haven't broken something. If you have major changes planned for ZCV and want to save the tests until then, that's fine too.

I think the randoms change looks fine to me, it's generating 1 octant rather than a full sphere, right? Do we have any concern about padding the randoms around the edge of the octant, or is that already taken care of by the fact that the lightcone is slightly inside the box?

Another possible change that comes to mind is making the randoms in float32 instead of float64. The memory usage here was a concern, right? I think the only precision concern is that $\cos(\theta) = 1$ for $\theta < 0.02\ \deg$ in float32. But our halo catalog Cartesian positions are in float32, so maybe we convert to float32 once we have the Cartesian randoms?

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