-
Notifications
You must be signed in to change notification settings - Fork 75
Feature/fkem refactor #1262
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
base: master
Are you sure you want to change the base?
Feature/fkem refactor #1262
Conversation
Pull Request Test Coverage Report for Build 20354464860Details
💛 - Coveralls |
pyccl/cells.py
Outdated
| *, | ||
| p_of_k_a=DEFAULT_POWER_SPECTRUM, | ||
| l_limber=-1, | ||
| ell_limber=-1, |
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.
Just a super quick comment. We cannot do changes to the API without bumping the major version; i.e. without going to CCLv4. Since we don't want that, you need to ensure that the changes respect the API.
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.
Hmm but this isn't a new feature -- it's a fix
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.
Because yes this is api but we use ell throughout the package so it's a fix in my opinion
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.
A fix that changes the current API, so we would have to bump CCL to v4 and maybe break a few working codes. I would open an issue and leave it for CCLv4.
But also in L22 fkem_Nchi=None. I would not change any of these. I haven't looked further so there might be similar issues in other parts of the code, it was a general comment.
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.
I can keep the old argument names (commit that)
But we really shouldn’t publish a paper on code that has missing docstrings or no unit tests for major components — that part isn’t an API change, it’s just basic scientific software quality. I’d like to at least add the documentation and tests now, even if the API cleanup has to wait for CCL v4.
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.
That's good. The tests and documentation are more than welcome, of course.
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.
ok, perfect. I will commit that and then raise an issue to fix it so we keep the ccl standard of arg naming etc
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.
btw -- just to understand -- you bumped the ccl version when fkem was initially coded in?
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.
I did not do the merge of that but I think it happened in v2. Since it would be a API compatible change, it would be a minor version change; i.e. from 2.X.Y to 2.X+1.0.
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.
@carlosggarcia done in 4e83abf
- Add low-ℓ safety logic in so shear–shear falls back
to non-linear Limber when the FKEM correction is unreliable.
- Remove temporary FKEM debug prints and extra benchmark debug output.
- Verified with:
pytest benchmarks/test_nonlimber.py::test_cells -s
4e83abf to
50f14b9
Compare
This PR refactors the FKEM non-Limber implementation from the legacy monolithic function into a modular, documented, and unit-tested pipeline, and wires it into
angular_cl. It addresses Refactor legacy FKEM non-Limber implementation into modular, documented code #1261.Closes #1261 and #1260
What changed
1. New modular FKEM pipeline
I introduced a structured FKEM module layout:
core.py: newnonlimber_fkemdriver that:n_consec_ellconsecutive multipoles belowlimber_max_error,ell_limber = -1, empty FKEM Cls, status = 0) if FKEM fails for a given configuration (and TELLS the user it happened).power_spectra.py:prepare_power_spectra:p_of_k_aandpk_linear(both string or bothPk2D),Pk2Dobjects and a callablepk,Nonetriplets and issues a clearCCLWarningwhen FKEM cannot be constructed, so that the core can degrade to Limber detreministicallytracers.py:build_tracer_collections:chi_grid.py:build_chi_grid:chi_min,chi_max, andn_chifrom tracer kernels (respectingfkem_chi_min/fkem_nchiwhen provided),chi_min == 0safely,dlnr+ effective paramschi_minstability by clamping overly small values to the tracer support and adds testssingle_ell.py+transforms.py:perform the per-ell FKEM computation and FFTLog-related pieces, returning:
2. Legacy FKEM moved and wrapped
The original
_nonlimber_FKEMimplementation is now inpyccl/nonlimber_fkem/legacy_fkem.pyaslegacy_nonlimber_fkem.The file was renamed and moved out of the top-level
pycclinto the dedicatednonlimber_fkempackage but kept intact so we can:3.
angular_clintegration and API tweakspyccl/cells.pynow calls the newnonlimber_fkemwhennon_limber_integration_method == "FKEM":The split between non-Limber and Limber parts is now derived from the length of the FKEM block:
so the FKEM driver fully controls how many multipoles are treated non-Limber.
I relaxed and clarified the FKEM sanity checks in
angular_cl:ell_limber <= 0is now treated as “no FKEM, pure Limber” (needed for the reference Limber test).ell_limber == min(ell), meaning FKEM can be active starting from the first multipole.ell_limbersmaller than the smallest requested ell:Minor API / naming cleanup: changed
fkem_Nchitofkem_nchito be PEP8-compliant and consistent with other parameters.4. Tests & docstrings
New regression tests in
pyccl/tests/test_fkem_legacy_vs_new.py:NC (with and without RSD)
nonlimber_fkemmatcheslegacy_nonlimber_fkemto tight tolerance for a simple NC auto-correlation.WL, known problematic configs (±IA)
legacy_nonlimber_fkemraisesCCLErrorfor some WL setups.[FKEM]CCLWarning, and returnsell_limber = -1, empty FKEM Cl array,status = 0(pure Limber).WL, successful legacy runs (±IA)
xfailed, as there is no reference Cl.High-level behaviour test in
test_cells_uses_fkem_for_low_ell_number_counts:ccl.angular_cl(..., ell_limber=0)produces a pure Limber reference.ccl.angular_cl(..., ell_limber=-1 or "auto"/default)uses FKEM at low ℓ for number counts and returns Cls consistent with Limber to a loose tolerance.angular_cl).I also added short docstrings to the new FKEM tests to make their intent explicit and easier to navigate.
Fixed a few relative imports and module-level docstrings in the FKEM package to keep things consistent and importable.
Behavioural changes
FKEM is now modular and testable, with clear separation of:
FKEM never silently disappears:
[FKEM]warnings and a documented fallback to pure Limber, instead of hidden behaviour.The semantics of
ell_limberare now:ell_limber <= 0: pure Limber (no FKEM),ell_limber > 0: FKEM is applied up to some ℓ, controlled bynonlimber_fkem(manual or auto),ell_limber == min(ell): allowed; FKEM active starting at the first multipole.Testing status
FKEM + cells tests (on my arm machine MacOS):
OMP_NUM_THREADS=2 pytest -vv pyccl/tests/test_fkem* pyccl/tests/test_cells.pyangular_clwiring test.Full
pyccltest suite:The following tests still error, but they are unrelated to this PR and reproduce failures already present on
main:pyccl/tests/test_bacco_lbias_power.pypyccl/tests/test_ept_power.py(missingunicodein newer Python)pyccl/tests/test_hmfunc.pypyccl/tests/test_lpt_power.pyThese look like scipy / python version-related issues and missing imports in other modules; I plan to address them in a separate PR.
flake8 complancy fixed in 6fc10a4
CI is failing now only for MacOS build and this s inherited from main. See this issue I explained here.