Tomographic pseudo-cl#221
Conversation
martinkilbinger
left a comment
martinkilbinger
left a comment
There was a problem hiding this comment.
Looks good, although there is some code duplication between pseudo_cl and pseudo_cl_tomo. Could we just have one function that internally checks whether tomography or not needs to be computed? Or set zbin=1 for non-tomography and have the same structure for all cases (tomography but in some cases with one bin)?
Done. I merged both functions with an API where the This is not optimal because columns are duplicated. |
cailmdaley
left a comment
cailmdaley
left a comment
There was a problem hiding this comment.
Structure looks right — the bin-pair loop, the factored two-field NaMaster primitives, and the shared pixelization are exactly the shape this wants. Since you flagged it as not-ready, here's a pass aimed at getting it there: four concrete bugs inline below, all with a common root — the tomographic path (and the map path's noise-bias branch) has not yet been executed, and no test exercises a real bin pair (everything still runs ("all","all")).
The one ask that closes all of this at once: a small end-to-end tomographic test — a tiny synthetic 2-bin catalog (or a #219 GLASS mock) through both estimator paths, checking that auto/cross spectra differ and E/B signs match the non-tomo path. It would have caught every finding below. Relatedly: this PR reads the bin column via cc[ver]["shear"]["tomo_bin_ids"] while #219 writes the literal column TOM_BIN_ID — neither PR adds the config entry wiring them together; worth adding it here so the GLASS validation loop actually connects.
— Fable, on behalf of Cail
| idx_rep=None, | ||
| wsp=None, | ||
| ): | ||
| cl_noise = np.zeros_like() |
There was a problem hiding this comment.
np.zeros_like() needs an argument — this raises TypeError on any auto-bin pair, including the plain non-tomographic ("all","all") case. Presumably np.zeros_like(cl_shear) as before the refactor.
|
|
||
| cl_noise = np.zeros_like(cl_shear) | ||
| rng = np.random.default_rng(self.cell_seed) | ||
| if tomo_bin_a != "all" and tomo_bin_b != "all": |
There was a problem hiding this comment.
These branches are swapped: real bin pairs (!= "all") take the unmasked full catalog, so every tomographic auto/cross spectrum computes the same full-catalog Cℓ; and the "all" case falls into the mask branch, where tomo_bin_id == "all" on an int column is all-False → empty catalog. Inverting the condition fixes both. (The catalog-side get_pseudo_cls_catalog has it right with is_tomography.)
Once inverted, note the assert above will also need isinstance(..., (int, np.integer)) — bins from np.unique on a FITS column are np.int64, which fails isinstance(x, int).
| factor = -1 if pol_factor else 1 | ||
| is_tomography = tomo_bin_a != "all" and tomo_bin_b != "all" | ||
| if is_tomography: | ||
| mask_tomo_a = catalog[params["tomo_bin_col"]] == tomo_bin_a |
There was a problem hiding this comment.
params["tomo_bin_col"] is never set anywhere (get_params_rho_tau doesn't know about it), so the catalog-path tomographic branch KeyErrors on first use. The map path reads the column name from cc[ver]["shear"]["tomo_bin_ids"] instead — worth picking one convention and threading it through both.
| cosmo_params=None, | ||
| blind=None, | ||
| compute_tomography=False, | ||
| force_run=False, |
There was a problem hiding this comment.
(Anchoring here — the actual line, 222, is outside the diff.) pol_factor changed meaning from bool ("apply the −1 flip?") to the literal multiplicative factor (docstring: int, default -1) — but the default at line 222 still passes True, which multiplies e2 by 1, silently dropping the sign flip in the fiducial path. Should be -1; maybe also assert pol_factor in (-1, 1) to catch stray bools from old call sites.
| ), "tomo_bin_a and tomo_bin_b must be either both 'all' or both integers." | ||
|
|
||
| shear_map = shear_map_e1 + 1j * shear_map_e2 | ||
| params = get_params_rho_tau(self.cc[ver]) |
There was a problem hiding this comment.
Minor: the refactor dropped survey=ver here (and in calculate_pseudo_cl_catalog), which calculate_pseudo_cl_eb_cov still passes — survey-specific overrides in get_params_rho_tau silently revert to defaults. Intentional?
There was a problem hiding this comment.
Ultimately, this should go away. This is hard-coding for some catalogue that is not relevant anymore and that should be removed in future PRs when the overall pipeline is stable.
There was a problem hiding this comment.
Pull request overview
This PR refactors the harmonic-space pseudo-Cℓ implementation by moving “elementary” pseudo-Cℓ utilities into sp_validation/pseudo_cl.py and updating the CosmologyValidation pseudo-Cℓ orchestration to support tomographic bin pairs, alongside corresponding test updates.
Changes:
- Added/expanded stateless pseudo-Cℓ primitives (pixelization, map building, noise-bias estimation, field/workspace helpers, iNKA covariance helper) in
src/sp_validation/pseudo_cl.py. - Updated the
CosmologyValidationpseudo-Cℓ mixin and core configuration to introduce tomography/force-run controls and to route computations through the new primitives. - Updated golden-value tests to the new API (notably separating pixel bookkeeping from number-density map construction and calling
apply_random_rotationfromsp_validation.pseudo_cl).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
src/sp_validation/tests/test_pseudo_cl.py |
Adjusts golden tests for the updated pseudo-Cℓ API (new pixel helper, updated signatures, direct import of apply_random_rotation). |
src/sp_validation/rho_tau.py |
Extends params to optionally include a tomographic-bin column reference. |
src/sp_validation/pseudo_cl.py |
Introduces/refactors core pseudo-Cℓ primitives to be callable outside cosmo_val. |
src/sp_validation/cosmo_val/pseudo_cl.py |
Integrates primitives into CosmologyValidation and adds tomographic bin-pair orchestration and new caching/output behaviors. |
src/sp_validation/cosmo_val/core.py |
Adds compute_tomography / force_run flags and a helper to enumerate tomographic bins. |
.gitignore |
Adds additional scratch paths to ignore. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I pushed an extra update of the iNKA covariance estimation for tomography. I think many things changed in the code and this could be the minimum for this PR but not enough to close the linked issue yet. I will start testing the GLASS mock update jointly with the pseudo-Cl measurement. |
|
I think I tiny unit test with a 2-bin setup would be nice to pin the current behavior and make sure there are no silly runtime errors :) |
|
I already have a set up, I will make the unit test when the more involved and relevant validation is done. Who can do more can do less |
sachaguer
merged commit e7d241a
into
feature/sp_validation-extend-to-tomography
|
I unintentionally merged the PR while bringing develop branch updates to the tomographic pseudo-cl branch. Apologies. I reverted the merge in |
I upgraded the harmonic space code to compute tomographic data vectors.
I modified the API related to any utility function in pseudo_cl.py. That way, any user of the repo can call the same "elementary" functions of
cosmo_valin notebooks or scripts outside ofcosmo_val. This summarises two separate behaviour:CosmologyValidation.pseudo_cl.pyscript.The current update passes CI after some tweaks to tests that were not compliant with the new API. Extra tests to validate the tomography approach would be valuable to write at some point.
The PR is not ready, I will keep working to have a similar thing for the covariance. But the current state of the PR allows to start validating the implementation of the tomographic GLASS simulations and the upgraded cosmo_val.