-
Notifications
You must be signed in to change notification settings - Fork 7
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
Ch/psestimate memory #96
base: master
Are you sure you want to change the base?
Conversation
This PR goes along with the one in driftscan. Asking also @tristpinsm and @sjforeman additionally to @jrs65 for review. Thanks! |
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.
Looks good to me! Perhaps @jrs65 or @tristpinsm should also take a look before the final approval.
draco/analysis/powerspectrum.py
Outdated
return ps | ||
|
||
|
||
class QuadraticPSEstimationXLarge(QuadraticPSEstimation): |
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.
Why does this need to be a separate class at all? Is there any reason this can't just replace the existing implementation? Or if not, merge them and change the behaviour to a config option, or even better, something that gets automatically determined based on the input data size.
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 mean, I could put them into one class and add a config option, but the two classes are fundamentally very different for the following reasons:
- the
QuadraticPSEstimationLarge
uses fromdriftscan
thePSMontelCarloLarge
class in which the clarray and the qestimator are MPIarrays parallelized over power spectrum bands. This is very different from the original code, in which clarray is gathered on all ranks and qestimators are calculated locally. - This class respects the fact that clarray and qestimator are parallelized over bands, therefore the sky data need to be pre-computed for all m first, as each rank needs all m's. This is also very different from the original code, where the sky data were handled parallel in m.
If we were to use a config option - then we would have one class but and if/else statement in the process method handling the two different calculations.
I personally thought a separate class is cleaner, but after all - you are the boss ;)
if mpiutil.rank0: | ||
print("m, time needed for quadratic pse", m, et - st) |
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 looks like left over debug statements. Either remove it, or at least improve the formatting.
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.
Would be good to keep, but agree about improving formatting
if mpiutil.rank0: | ||
print("m, time needed for quadratic pse", m, et - st) |
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.
Would be good to keep, but agree about improving formatting
et = time.time() | ||
if mpiutil.rank0: | ||
print("m, time needed for quadratic pse", m, et - st) |
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.
et = time.time() | |
if mpiutil.rank0: | |
print("m, time needed for quadratic pse", m, et - st) | |
et = time.time() | |
self.log.debug(f"{m=}, time needed for quadratic pse {et - st}") |
A quadratic estimator that is distributed over power spectrum bands.
feat(QuadraticPSEstimationXLarge):