Conversation
Pull Request Test Coverage Report for Build 21326062937Details
💛 - Coveralls |
nikosarcevic
force-pushed
the
feature/baryons_bhm_fedeli
branch
from
a07e69b to
423171e
Compare
carlosggarcia
left a comment
carlosggarcia
left a comment
There was a problem hiding this comment.
Thanks for the work, @nikosarcevic. This is great!
This is a massive PR so I think it might be better if we do multiple iterations over a few files. I've started with the API ones. When we settle on those, I will go into the files with the actual implementation.
A few comments:
- The CI is failing because the coverage level of the code has gone down. You need to write more unit tests to test all new additions to the code; i.e. as many lines as possible, which is what coveralls checks.
- I think the policy is to put the notebooks in the CCLX repo. So you should move them from this PR.
- If you come up with a way of making this PR shorter, that will make our lives easier and the lives of those who have to look into this in the future!
pyccl/baryons/fedeli14.py
Outdated
| The operation applied is: | ||
|
|
||
| .. math:: | ||
| P_{\rm out}(k,a) = P_{\rm in}(k,a)\,B_{\rm fedeli}(k,a). |
There was a problem hiding this comment.
Change B -> f, for consistency with the other modules.
There was a problem hiding this comment.
done in c5812c5
(sorry about CI failing, am trying to fix it, i tihnk it is osx-arm64 standard nightmare suddenly)
| - If ``renormalize_large_scales=True``, the boost is rescaled (per | ||
| scale factor) so that its mean over ``k <= k_renorm_max`` is unity. | ||
| - For ``a < a_min`` (currently hard-coded to 0.1 in the application | ||
| step), the boost is forced to unity. |
There was a problem hiding this comment.
Is this also enforced in Fedeli+2014 or is this an addition?
There was a problem hiding this comment.
this is an addition in the wrapper
fedeli4 builds the baryonic halo model so that the large-scale limit is physically correct (e.g. the 2halo term recovers linaer theory and the k->0 limits recover the correct mass fractions), but it does not prescribe an explicit renormalization of a boost ratio to unity over a finite low-k range
the `renormalize_large_scales`` option is basically a numerical safeguard to ensure the ratio f(k,a) -> 1 on large scales when dividing by a chosen reference spectrum, given finite grids and interpolation effects.
Elisa and I had this problem on large scales and this is a solution I found.
in a similar fashion, forcing the boost to unity for a < 0.1 is bascially a defensive cutoff to avoid extrapolating the model outside its intended regime and is not also explicitly imposed in fedeli14
pyccl/baryons/fedeli14.py
Outdated
| "star_x_delta", "star_alpha", "density_mmin", "density_mmax", | ||
| }: | ||
| val = float(val) | ||
| if key in {"n_m"}: |
pyccl/baryons/fedeli14.py
Outdated
| k: float | FloatArray, | ||
| a: float | FloatArray | ||
| ) -> float | FloatArray: | ||
| r"""Evaluate the Fedeli14 boost factor ``B(k,a)``. |
There was a problem hiding this comment.
Same as above B(k,a) -> f(k,a)
pyccl/baryons/fedeli14.py
Outdated
| for i, aval in enumerate(a_use[:, 0]): | ||
| out[i, :] = bhm.boost(k=k_1d, a=float(aval), pk_ref=self.pk_ref) | ||
|
|
||
| # We need to enforce B(k->0)=1 by renormalizing on large scales. |
| per-a.""" | ||
| b = BaryonsFedeli14(renormalize_large_scales=False) | ||
|
|
||
| calls: list[tuple[float, np.ndarray, str]] = [] |
| @@ -0,0 +1,312 @@ | |||
| """Unit tests for `pyccl.baryons.fedeli14`.""" | |||
|
|
|||
There was a problem hiding this comment.
Two general comments:
- I think it's clever the way you use monkeypatch. I'd try to define a single DummyBHM and use the same for all tests. That would shorten the file and make it a bit easier to read, so that I don't have to re-think in each test what this dummy class is doing.
- Although I guess you will test the output of the actual Fidelly14 boost computation. I think there is value if you add a single test that actually test that you get the right boost, without DummyBHM class.
|
|
||
| # sanity: grids unchanged | ||
| a0, lk0, _ = pk.get_spline_arrays() | ||
| assert np.allclose(a_arr, a0) |
There was a problem hiding this comment.
I think it's more explicit to use assert a0 == pytest.approx(a_arr, rel=1e-5). Numpy can change the relative/absolute tolerances and break all our tests.
| a0, lk0, _ = pk.get_spline_arrays() | ||
| assert np.allclose(a_arr, a0) | ||
| assert np.allclose(k_arr, np.exp(lk0)) | ||
|
|
There was a problem hiding this comment.
All the lines below are a copy of the above?
| assert np.allclose(pk_out[1:, :], 20.0) # boosted by 10 for a>=0.1 | ||
|
|
||
|
|
||
| def test_incl_bary_eff_fixes_absurd_log_flag_data( |
3 participants
This PR adds the Fedeli 2014 baryon halo model to CCL and integrates it into the
baryonsmodule.Included are:
a full fedeli14 baryon halo–model implementation under
pyccl/baryons/fedeli14_bhm/, including:BaryonHaloModelclass (baryon_halo_model.py)a public-facing baryons API in
pyccl/baryons/fedeli14.py, exposing fedeli14 as a multiplicative baryonicboost that returns baryon-modified power spectra
Advanced users can directly instantiate and customize the
BaryonHaloModelto experiment with halo ingredients and internal modelchoices.
Example notebooks (under
fedeli14_bhm/examples/) demonstrating:BaryonHaloModelI also include unit tests covering each fedeli14 component/script.
Note that:
pytest pyccl/tests/test_baryons_fedeli14*)this pr is built off of current ccl master
this closes #1276