-
Notifications
You must be signed in to change notification settings - Fork 18
halo light cones: reduce memory usage in prepare_sim #173
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
Conversation
abacusnbody/hod/prepare_sim.py
Outdated
| allpos[index_bounds], r=rad_outer, workers=nthread | ||
| ) | ||
| # this is the true number of randoms | ||
| for ind in np.arange(len(index_bounds)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be for ind in range(len(index_bounds)), right? No need to construct the full array with np.arange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks for catching! Also changed it in one more place
| # repeat until condition satisfied | ||
| while count < len(index_bounds) * rand_final: | ||
| # generate randoms in L shape | ||
| randpos, randdist = gen_rand( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test failure is due to this function calling np.random functions a different number of times than before, causing subsequent np.random calls to return different results. Maybe we should decouple this RNG with later RNG?
In other words, can this function be modified to accept an RNG object (created by np.random.default_rng(seed)) instead of using np.random directly? And then we reuse the RNG object for all calls to this function, and a different RNG object for other parts of the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! Included rng object set at newseed that is passed to that function -- hopefully it passes the tests now (though I worry that we also had a check for Menv so it might still fail because the values are different?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will fix the tests, because previously the RNG for the randoms and Menv was coupled, and now we're decoupling it. But at least it will prevent the tests from breaking in the future.
This lets us give informative error messages that include the field name, and reduces repetition.
But separately from the seed used for the rest of the randoms.
…utils into mem_prepare_sim Please enter a commit message to explain why this merge is necessary,
Attempting a memory improvement of the compute environment script in
prepare_sim.py.Addressing issue #143