Power spectrum responses for SSC#1134
Conversation
Pull Request Test Coverage Report for Build 16628354402Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
@RyoTerasawa can I check what the status of this PR is? |
|
I am waiting for someone to review the code. I haven't asked for a specific person yet, but maybe I should ask a member of the MCPCov group to review it. I am happy to explain the details if needed. I am also working on writing the unit test and benchmark test to be included in this PR. |
|
@RyoTerasawa @YueNan-c, maybe you want to also implement the DarkEmulator here: https://github.com/LSSTDESC/CCL/blob/master/pyccl/emulators/cosmicemu_pk.py. Are the unit tests and benchmark ready? |
|
@RyoTerasawa @carlosggarcia can I check what the status of this is? |
|
@carlosggarcia @damonge |
Thank you for the through review. Now this SSC branch is up to date with the master branch, and 33 commits ahead of the master branch. |
merge from master branch
Merge changes in master
carlosggarcia
left a comment
There was a problem hiding this comment.
I would re-add the lk_arr and a_arr options. @damonge can you check the comments above for the integrals?
| raise ValueError("you must provide arrays") | ||
| # Set k and a sampling from CCL parameters | ||
| if a_arr is None: | ||
| a_arr = get_pk_spline_a() |
There was a problem hiding this comment.
Thanks! I would use the same code that was there before, just in case there is a subtle difference that we are missing
if lk_arr is None:
status = 0
nk = lib.get_pk_spline_nk(cosmo.cosmo)
lk_arr, status = lib.get_pk_spline_lk(cosmo.cosmo, nk, status)
check(status, cosmo=cosmo)
if a_arr is None:
status = 0
na = lib.get_pk_spline_na(cosmo.cosmo)
a_arr, status = lib.get_pk_spline_a(cosmo.cosmo, na, status)
check(status, cosmo=cosmo)
There was a problem hiding this comment.
I modified so that the spline uses the same code that was there before.
carlosggarcia
left a comment
There was a problem hiding this comment.
LGTM! I'd wait for @damonge to check whether some of the integrals are duplicating some existing functionality before merging.
damonge
left a comment
There was a problem hiding this comment.
I trust the functional contents of this PR are fine since they've been reviewed by @carlosggarcia . I don't mind that the halo model integrals are carried out outside of the current halo model (we could incorporate this later if this module becomes more embedded in the pipeline). However, I have a few requests about the structure of the new module itself:
- First, change the name of the module to
pk_responserather thanpkresponse, for clarity. - Second, we need better names for all the new functions, so they are easily identifiable. My suggestion is as follows:
- Use a structure of the form
resp_<pk_kind>_<method>, where<pk_kind>isPmm,Pgmetc., and<method>is the method used to calculate the response. - With this in mind, the new functions you have added should read
resp_Pmm_hresponse(for example),resp_Pgm_darkemuetc.
- Use a structure of the form
I also have an overarching comment: presumably we would like to use the contents of this PR to improve the way we compute the SSC covariances. However, right now the methods you have implemented are completely orphan, and cannot be used for this. The way things currently stand, the SSC contribution computed here automatically assumes the PT approach to the power spectrum response.
Before we move forward, can I ask what the plans are for embedding the new responses in the existing SSC infrastructure? I'm asking because the structure of the code added by this PR should be influenced by how we will use this code later on.
| return dpk12 | ||
|
|
||
|
|
||
| def darkemu_Pgm_resp( |
There was a problem hiding this comment.
This function needs to be properly documented (include a description of all arguments). It would also be good if its name matched the structure of the previous function (i.e. Pgm_resp instead of darkemu_Pgm_resp, since the previous function is Pmm_resp).
There was a problem hiding this comment.
If you want to specify that the response is calculated with darkemu, I would do this at the end of the name, not at the start. I'll propose a naming scheme at the end of this review.
| nP = nth_mat * np.array(Pth) | ||
|
|
||
| # Eq. A7 | ||
| Pgm = integrate.romb(dprof_dlogM * nP, dx=dlogM, axis=0) / ng |
There was a problem hiding this comment.
I'm ok leaving this outside of the halo model framework for now, since these methods are all quite specific. If we ever wanted to generalise this, I would indeed suggest that we incorporate this fully within the existing halo model module.
| return dpk12 | ||
|
|
||
|
|
||
| def darkemu_Pgg_resp( |
There was a problem hiding this comment.
As before, this needs proper documentation and homogenise the function names.
| Pgg_2h_int = np.array(Pgg_2h_int) | ||
|
|
||
| # Eq. A12 | ||
| _Pgg_2h = integrate.romb( |
| G_prof = ( | ||
| +2.0 | ||
| / 3.0 | ||
| * integrate.romb( |
|
@carlosggarcia @damonge Thank you for the through review.
|
carlosggarcia
left a comment
There was a problem hiding this comment.
Two missing arguments in the documentation.
Also, @damonge, we will need your approval
| which the response is calculated. | ||
| a_arr (array): an array holding values of the scale factor | ||
| at which the response is calculated. | ||
|
|
|
@damonge can you check if you are happy with this now? |
In pyccl/pkresponse.py, I implemented the functions to calculate power spectrum responses to the super-survey modes which are responsible for super-sample covariance (SSC), based on arXiv:2310.13330.
We approximate the power spectrum growth response to the super-survey modes by its growth response to the Hubble parameter.
For the galaxy-matter and galaxy-auto power spectra, the halo statistics (halo-matter/halo-auto power spectrum, mass function) are calculated by DarkEmulator.