-
Notifications
You must be signed in to change notification settings - Fork 1
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
Ssc fsky #115
Ssc fsky #115
Conversation
…ust still add tests
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.
Great work. Just some small changes to avoid duplicating code.
nfw = ccl.halos.HaloProfileNFW( | ||
mass_def=mass_def, concentration=cM, fourier_analytic=True | ||
) | ||
hmc = ccl.halos.HMCalculator( |
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.
We should move this out of this function. That way we can reuse whatever it's already computed in each hmcalculator. But we can open an issue later.
from .covariance_builder import CovarianceFourier | ||
|
||
|
||
class FourierSSCHaloModelFsky(CovarianceFourier): |
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 class is basically the same as FourierSSCHaloModel
except for a few lines. I would not create a new class but check whether fsky is defined in the SSC configuration that you retrieve in the __init__
. If fsky is there, then you compute the sigma2_B with the fsky method; otherwise with the mask.
Alternatively, I would add a method to the class that returns the sigma2_B, inherit from this class in your FourierSSCHaloModelFsky
and overwrite that method.
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.
Right, I agree. I can make some modifications to reduce the code duplication. Should I make this on a new branch then?
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.
As you prefer. You can create a new branch and merge it onto this one or work directly on this one. I would probably do the later. It's an easy modification so in one or two commits should be done.
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.
Almost there!
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.
Almost there! One last question
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.
LGTM!
This PR includes a simple extension of the SSC covariance using the Fsky approximation. The user will no longer require a mask to compute this term to the non-Gaussian covariance, also enabling a full comparison between the approximation and a mode-coupling formalism.