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

Ssc terasawa #991

Closed
wants to merge 22 commits into from
Closed

Ssc terasawa #991

wants to merge 22 commits into from

Conversation

RyoTerasawa
Copy link

I have implemented a function Tk3D_SSC_Terasawa22 to compute super-sample covariance into the tk3d.py.

Add the function Tk3D_SSC_Terasawa22 for computing super-sample covariance. Also add some module to import.
@RyoTerasawa RyoTerasawa marked this pull request as ready for review September 29, 2022 05:25
Copy link
Contributor

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. It looks good to me (modulo the lint errors that GitHub is complaining about). Just a few minor comments.

In addition, it'd be great if you can add an unit test for this function (see https://github.com/LSSTDESC/CCL/blob/master/pyccl/tests/test_tkkssc.py for reference). If you were comparing this Tk with some other code, you could also add some benchmark (see e.g. https://github.com/LSSTDESC/CCL/blob/master/benchmarks/test_covariances.py). Or maybe with the simulations that you have? What do you think @damonge?

pyccl/tk3d.py Outdated

.. math::
\\frac{\\partial P_{mm}(k)}{\\partial\\delta_L} =
\\left(1 + \\frac{26}{21}T_{h}(k) -\\frac{1}{3}\\frac{d\\log P_{mm}(k)}{d\\log k}\\right)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a quick look at Terasawa+2022 and couldn't find where the -1/3... term is coming from. If it's easy to derive or I have missed it, it'd be great to point to the Eq where it is (or that you have to use to get it) for future reference.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The power spectrum response to the background density is consist of two components: growth response and dilation response (and change in the reference density). Terasawa+2022 focused on the growth response and the dilation term is simply the derivative of the power spectrum with respect to the wave number k. The total power spectrum response (including growth and dilation term) is found in Eq. 46 and 47 of Li, Hu, and Takada 2014a (https://arxiv.org/abs/1401.0385), for example.

pyccl/tk3d.py Outdated
extra_parameters=extra_parameters)

# Growth factor
Dp = background.growth_factor_unnorm(cosmo_hp,a_arr)
Copy link
Contributor

@carlosggarcia carlosggarcia Sep 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the background dependency and use directly cosmo_hp.growth_factor_unnorm(a_arr). Same in the next line

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out. I have removed the background dependency as you mentioned.

@damonge
Copy link
Collaborator

damonge commented Sep 29, 2022

Yes, besides the unit tests Carlos mentions, it'd be important if you can add any type of validation you've done (e.g. against simulations) as a benchmark test.

@RyoTerasawa
Copy link
Author

I am going to add a unit test and a benchmark test later.

@coveralls
Copy link

coveralls commented Oct 6, 2022

Pull Request Test Coverage Report for Build 4290191698

  • 50 of 62 (80.65%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 97.32%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyccl/tk3d.py 50 62 80.65%
Totals Coverage Status
Change from base Build 3862532670: -0.2%
Covered Lines: 4902
Relevant Lines: 5037

💛 - Coveralls

@nikfilippas nikfilippas mentioned this pull request Mar 11, 2023
@RyoTerasawa
Copy link
Author

The changes in this PR is now included in
#1134

@RyoTerasawa RyoTerasawa closed this Dec 4, 2023
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.

5 participants