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

power spectrum module optimization and parallelization #102

Merged
merged 8 commits into from
Jul 14, 2023
Merged

Conversation

lgarrison
Copy link
Member

Some optimization, parallelization, and Numba-fication of various parts of the power spectrum module. Benchmark scripts used to produce the timings in the ZCV paper are included.

@lgarrison
Copy link
Member Author

@boryanah Here are the changes we talked about. It's about half optimization and half refactoring. I tried to rename some things that were confusing to me, but let me know if I misunderstood anything. I also tried to cut down on the number of arguments and return values in some places. For example, I changed calc_power() to return an Astropy Table; let me know if you think that makes sense.

@lgarrison lgarrison marked this pull request as ready for review July 12, 2023 22:05
@lgarrison lgarrison requested a review from boryanah July 12, 2023 22:07
@boryanah
Copy link
Collaborator

That looks great to me! The optimizations make sense (I am sorry I didn't implement the normalization one myself). The astropy table for the power spectrum with the meta data is great, and I think it's a good solution for outputting a single object that contains some useful information about the simulation. I think it makes sense why you got rid of some of the del X; gc.collect() (they probably weren't doing anything as the variables were passed externally rather than locally defined). I also like the variable and function name changes, and I think they make more sense.

@lgarrison
Copy link
Member Author

Yeah, that's exactly right about the gc.collect(). The other reason was that garbage collection was making the timings really noisy; there might be one or two that could go back in, but for the most part I think they weren't doing anything. And if we really wanted to save memory, there are other ways: using an in-place FFT (pyfftw supports this, not sure if scipy.fft does), and adding on-the-fly offsets for the interlacing calculation (right now it makes a whole copy of the input data).

@lgarrison lgarrison merged commit b0ae7b7 into main Jul 14, 2023
8 checks passed
@lgarrison lgarrison deleted the pk_bench branch July 14, 2023 14:10
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.

None yet

2 participants