refactor(cosmo_val): god-class → tested, primitive-backed package#207
Draft
cailmdaley wants to merge 23 commits into
Draft
refactor(cosmo_val): god-class → tested, primitive-backed package#207cailmdaley wants to merge 23 commits into
cailmdaley wants to merge 23 commits into
Conversation
…-export) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Converting cosmo_val from a module to a package moved every file one level deeper, so single-dot relative imports to top-level siblings now resolve inside the package. Point them up a level: - ..b_modes (core, pure_eb, cosebis) - ..cosmology (core, pseudo_cl) - ..rho_tau (psf_systematics, pseudo_cl) Re-export cs_plots in __init__ from its origin (cs_util.plots) rather than from core, which no longer needs the alias after the plotting methods moved to their mixins. Preserves the sp_validation.cosmo_val.cs_plots attribute path. Package now imports cleanly; test_cosmo_val + test_b_modes: 18 passed, 1 skipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…module The demo called cosmo_val.hsp_map_logical_or, but that function has only ever lived in plots.py — the call raised AttributeError. Import it from its home. (Pre-existing bug surfaced by the cosmo_val package split.)
…eserving) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…m_one_covariance directly Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a private CosmologyValidation._output_path(*parts) that returns
os.path.abspath(os.path.join(self.cc["paths"]["output"], *parts)), and
route the 29 abspath-wrapped output-path constructions across the mixin
modules (catalog_characterization, pseudo_cl, psf_systematics,
real_space) through it.
Behavior-preserving: only the os.path.abspath(f"{output}/...") sites are
converted, where abspath(join(base, suffix)) is byte-identical to
abspath(f"{base}/{suffix}") for every suffix shape in the package
(verified, incl. embedded subdirs and trailing slashes). Raw f-strings
that are not abspath'd (psf_systematics output_dir handoffs to
shear_psf_leakage) are intentionally left as-is to avoid changing the
string they pass to external tools.
Value-drift gate green: 18 passed, 1 skipped.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… it (re-bless pins ~ULP) chi2_and_pte now computes chi2 via np.linalg.solve (not forming C^-1) and PTE via stats.chi2.sf (not 1 - cdf), the numerically proper forms: solve is more stable/cheaper than an explicit inverse, and the survival function avoids catastrophic cancellation in the low-PTE tail. The two hand-inlined C_l^BB chi2/PTE sites in cosmo_val (core.summarize_bmodes, pseudo_cl.plot_pseudo_cl) already used solve+sf, so they now route through the single chi2_and_pte primitive with no numerical change; their local scipy imports are dropped. No pinned value shifted (the value-drift pins flow through the independent b_modes Hartlap path), so no re-blessing was required. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r calibration Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…per caller The shared helper applied the DES R11/R22 branch to both callers, but calculate_aperture_mass_dispersion historically used scalar R for every version. Add a des_branch flag so each caller keeps its exact prior behavior (2pcf: DES branch; aperture-mass: scalar R). The asymmetry is flagged in the docstring as a likely latent bug for a deliberate, separate fix.
calculate_aperture_mass_dispersion used scalar R for every version, including DES, while calculate_2pcf used the catalog-averaged R11/R22 for DES. That asymmetry was a latent bug; _calibrated_g now applies the DES branch identically for both callers. Untested path (no DES fixture) — value-drift suite unaffected and green.
…n the mixin Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…_cl.py, thin the mixin Move the stateless pseudo-Cl computation out of PseudoClMixin into named free functions in a new top-level sp_validation/pseudo_cl.py (mirrors b_modes.py / rho_tau.py): make_namaster_bin, get_n_gal_map (weights kwarg, subsumes the glass_mock unweighted twin), apply_random_rotation (rng injectable; default preserves the no-arg np.random.seed() noise-debiasing behavior), and the map-/catalog-based estimators get_pseudo_cls_map / get_pseudo_cls_catalog. pseudo_cl_geometry centralizes the (lmin, lmax, b_lmax) triple. The mixin methods are now thin wrappers (load via self -> call primitive), public names/signatures unchanged so call sites do not move. glass_mock's get_n_gal_map delegates to the primitive via a lazy import, preserving its CAMB-only import-time independence from the harmonic stack. Pins (test_pseudo_cl.py) green; value-drift gate (test_cosmo_val + test_b_modes) 18 passed / 1 skipped; glass_mock tests green; actual cross-module import verified in the container. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…opies Repoint the GLASS-mock harmonic stats at the new src/sp_validation/pseudo_cl.py primitives so the bandpower binning can no longer drift from the estimator the rest of the package uses. - powspace_bins: deleted. Its square-root bandpower spacing is exactly pseudo_cl.make_namaster_bin(..., "powspace", power=0.5) on the pseudo_cl_geometry(nside) range. Replaced by a thin _mock_powspace_bin wrapper that returns the same (bin, ell_eff, lmax, b_lmax) 4-tuple the mock helpers consume, now sourced from the shared primitive. Verified bitwise-identical binning (ell_eff and per-bin ell membership) across nside/lmin/n_bins configs. - get_n_gal_map: already delegates to the weighted primitive with weights=None (unweighted counts); left as the count-default twin used by the map estimator. - compute_two_point_cl / compute_two_point_cl_map: kept their own NaMaster field construction (documented) — the mock field is UNWEIGHTED and these return the COUPLED pseudo-Cl alongside the decoupled one with the ell axis prepended, which the primitives' (ell_eff, cl_all, wsp) contract does not expose. They now take their binning and galaxy-count map from the shared primitives. Value-drift gate green (18 passed, 1 skipped); full non-slow suite 136 passed, 1 skipped, 1 xpassed, only the two known-environmental failures (test_bmodes_workflow_dry_runs, test_configured_paths_exist_on_candide). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
apply_random_rotation called np.random.seed() with no argument, re-seeding from OS entropy on every call, so the pseudo-Cl noise-debiasing realizations were non-reproducible run-to-run. Drop the bare seed; the primitive now uses its rng argument (default_rng() when None), and the debiasing + covariance sampling paths thread a generator seeded from a new CosmologyValidation cell_seed (default 8192). Same numerical scheme, now reproducible. The test_pseudo_cl reproducibility test replaces the old non-determinism guard.
cailmdaley
changed the title
5 tasks
cailmdaley
requested review from
martinkilbinger and
sachaguer
and removed request for
martinkilbinger
Collaborator
Author
|
@sachaguer you may want to take a look at the pseudo_cl random seed behavior and where to integrate in the namaster utils, although the latter doesn't have to be part of this PR! |
sachaguer
reviewed
Jun 24, 2026
sachaguer
left a comment
sachaguer
left a comment
Contributor
There was a problem hiding this comment.
I checked the seeding for the pseudo_cl and it looks fine to me.
Some of the namaster_utils are in pseudo_cl.py. Functions could still be broken into more elementary pieces but it is good as is and can be kept for a future PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Turns the 3,700-line
CosmologyValidationgod-class into a clean, testedcosmo_val/package backed by shared primitive modules. Behavior-preserving throughout, except three clearly-committed bug fixes (below). Built in reviewable layers — review per commit or per section:cosmo_val.py→ acosmo_val/mixin package, one module per diagnostic (pseudo_cl,psf_systematics,real_space,catalog_characterization,pure_eb,cosebis,core).CosmologyValidationcomposes the mixins; every call site is unchanged. The 85 methods are conserved exactly._output_path/_binning/_cprinthelpers,get_cov_from_onecovfolded intostatistics.cov_from_one_covariance,chi2_and_ptestandardized tosolve+sf.survey.py, and the NaMaster estimator → a new top-levelpseudo_cl.py.glass_mock's drifted estimator copies now route through the shared primitives.test_pseudo_cl.pyadds the first numeric pins for the pseudo-Cℓ estimator (a Paper II deliverable that had none) — golden values for the three binning schemes, the occupancy map, and the map/catalog EE·EB·BB spectra.The architecture this lands
cosmo_val/is the stateful orchestrator (holds config/paths/results, loads catalogs, sequences diagnostics, caches, plots, saves). The top-level modules (b_modes,rho_tau,cosmology,statistics,survey,pseudo_cl) are the stateless primitives that analysis scripts also import directly — which is why they live outside the class.Bugs fixed (each its own commit)
demo_create_footprint_maskcalledcosmo_val.hsp_map_logical_or, which lives inplots.py— repointed.Rfor every version while 2pcf used the catalog-averagedR11/R22; unified so DES is calibrated the same way in both.apply_random_rotationcallednp.random.seed()with no argument, re-seeding from entropy every call. Now threads a generator seeded from a newCosmologyValidation.cell_seed(default 8192): same Monte-Carlo scheme, reproducible run-to-run.Verification
ruff check src/sp_validationclean; all modules import (the relative-import depth is exercised, not just linted). Value-drift (test_cosmo_val,test_b_modes) green; the newtest_pseudo_clpins pass; full non-slow suite: 136 passed.Stacked on #206. Draft for review.
— Claude on behalf of Cail