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

Enhancement to the perturbative modelling #1002

Merged
merged 3 commits into from
Dec 19, 2022
Merged

Enhancement to the perturbative modelling #1002

merged 3 commits into from
Dec 19, 2022

Conversation

tilmantroester
Copy link
Contributor

This PR contains enhancement to the perturbative modelling in CCL to make its integration in firecrown easier.

  • Allow skipping of calling ptc.update_pk in get_pt_pk2d

@coveralls
Copy link

coveralls commented Oct 19, 2022

Pull Request Test Coverage Report for Build 3296719831

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.0005%) to 97.503%

Files with Coverage Reduction New Missed Lines %
pyccl/nl_pt/power.py 1 98.74%
Totals Coverage Status
Change from base Build 2917968053: 0.0005%
Covered Lines: 4841
Relevant Lines: 4965

💛 - Coveralls

@damonge
Copy link
Collaborator

damonge commented Oct 19, 2022

I'm happy to implement this fix for now, but the fact that we have to do it seems like a design flaw to me.

So looking forward, since we'll be able to break API with v3, how does the following scheme sound?
a) We make the PTCalculator also take in an array of scale factors as input (right now we fix the k sampling, so fixing the a sampling would seem ok to me too).
b) Then, we make the update_pk method depend on a cosmology and an input pk. It will then call the necessary fastpt functions, as it now does, and will evaluate and store the growth factor at the internal set of as.
c) We then remove any cosmology dependence from the get_pt_pk2d function, which will now only depend on tracers and a PTCalculator.

@tilmantroester
Copy link
Contributor Author

That sounds good to me. One thing I noticed was that for nonlin_pk_type == 'nonlinear' and requesting a matter-matter power spectrum, get_pt_pk2d evaluates the existing non-linear power spectrum at the ptc k sampling and then recreates a new Pk2D object (with is_logp=False). Since the ptc k sampling differs from the internal P(k) splines, the extrapolation (or lack thereof) then is different for the Pk2D object you get back than the CCL-internal power spectrum, even though they both use the same underlying P(k).

@tilmantroester tilmantroester marked this pull request as ready for review December 19, 2022 17:31
@tilmantroester
Copy link
Contributor Author

Could we merge this for now? This is currently holding up the firecrown PR.

@marcpaterno
Copy link

To close the Firecrown PR in question, we will need a release on conda forge with this addition. When is such a release feasible?

@damonge damonge merged commit 2e957aa into master Dec 19, 2022
@damonge damonge deleted the pt_enh branch December 19, 2022 19:24
@damonge
Copy link
Collaborator

damonge commented Dec 19, 2022

@marcpaterno let me try to tag a release and see if we can get it up within the next couple of days

@marcpaterno
Copy link

Thanks David!

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.

None yet

4 participants