Repository navigation
cosmo_val + Snakemake migration to the SACC writers (PRD #241 row 4) - #251
Merged
Merged
Conversation
This was referenced Jul 10, 2026
Closed
Add sp_validation.sacc_io: the writer/reader layer for the two-file SACC
layout that becomes the package's standard data-product format.
{version}.sacc analysis vector — NZ tracers, coarse xi+/-, pseudo-Cl
(EE/BB/EB) with a shared BandpowerWindow, COSEBIs,
pure E/B, rho/tau PSF diagnostics; one FullCovariance
assembled block-diagonally (zero cross-blocks).
{version}_xi_fine COSEBIs/pure-EB integration input — same NZ tracers,
fine-grid xi+/-, DiagonalCovariance from TreeCorr
varxip/varxim.
Covariance order is point-insertion order (SACC preserves it bitwise
through FITS). Writers insert in the canonical order — xi+ then xi-, Cl
(ee, bb, eb), COSEBIs (all En then all Bn), pure E/B in _EB_KEYS order
(xip_E, xim_E, xip_B, xim_B, xip_amb, xim_amb, matching
b_modes.calculate_eb_statistics), rho, then tau — but readers never assume
global order: every getter resolves indices through s.indices(dtype,
tracers, **tags). assemble_covariance validates that blocks are contiguous,
ascending and tile the data vector exactly, failing loud otherwise.
Custom data types (pure E/B, rho, tau) all parse under
sacc.parse_data_type_name. Tag filters are plain kwargs; the tags={...}
form silently selects nothing and is never used.
Test suite (test_sacc_io.py, all synthetic and fast): per-writer
round-trips (arrays/tags/windows/NZ bitwise), covariance block alignment
and zero cross-blocks, assemble_covariance failure modes, DiagonalCovariance
round-trip, extract() sub-covariance alignment, a tomographic multi-pair
case, reader/writer mirroring on a mixed file, and the end-to-end two-file
layout. 20 passed.
Co-Authored-By: Claude Opus <noreply@anthropic.com>
Fresh-eyes review caught a correctness bug: readers re-sorted selections by
theta/ell/n, but covariance blocks and bandpower windows stay in insertion
order. On a non-ascending grid the reader output silently desynchronised
from its covariance, and get_pseudo_cl returned sorted cl arrays against
unsorted window columns — internally inconsistent within one return tuple.
Fix by construction, not by sort:
- Writers validate their grids. add_xi/add_pure_eb/add_rho/add_tau require
strictly ascending theta; add_pseudo_cl requires strictly ascending
ell_eff (add_cosebis is inherently safe — it enumerates the mode index).
Out-of-order grids raise a loud ValueError naming the argument.
- Readers drop the sort entirely and return in s.indices (insertion) order,
so every getter is covariance- and window-aligned for ANY file, and
ascending for canonical files. The _sorted_* helpers are replaced by
plain insertion-order accessors (_mean/_tag).
Also:
- _pair normalises (i, j) -> sorted, so get_xi(s, (1, 0)) addresses the same
symmetric shear-shear pair as (0, 1) instead of a silent empty read.
- Module docstring documents the tomographic ξ covariance ordering:
insertion is pair-major ([pair0 xip; pair0 xim; pair1 xip; …]), supplied
to assemble_covariance as one contiguous block matching add_xi call order;
type-major converters (DES 2pt-FITS) permute explicitly via s.indices.
- extract() docstring states tracers takes SACC names, not integer bins.
New tests (26 total, was 20): writers reject non-ascending theta/ell;
(1, 0) == (0, 1) round-trip; a 3-pair tomographic ξ covariance assembled as
one contiguous pair-major block with per-pair sub-blocks recovered via
extract(); get_pseudo_cl window column j <-> ell_eff[j] via window_ind tags.
Co-Authored-By: Claude Opus <noreply@anthropic.com>
The two-file split (analysis + {version}_xi_fine.sacc) was premised on a
10000-bin fine grid; the production operating point (Paper II B-modes) is
1000 bins, where a dense per-pair fine covariance block is ~32 MB and the
CosmoCov integration-binning covariance — which feeds pure-E/B and COSEBIs
error propagation — has a natural home as a BlockDiagonal block alongside
the analysis blocks. Layout test replaced with the one-file end-to-end
case (dense fine block, extract() sub-covariance alignment, zero
cross-blocks) plus a varxi-diagonal fallback test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019R5eiy11Lihkgn4MKufXSp
…-forward) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019R5eiy11Lihkgn4MKufXSp
…date_statistic PRD #241 §4 (Mocks vs data): save() requires type='data'|'mock' and stamps it into metadata; load() raises on type='data' files lacking the concealed=True blinding stamp, so skipping the blind can never silently expose real data. Mocks load freely. allow_unblinded=True is the loud escape hatch reserved for the blinding/unblinding tooling. merge() wraps sacc.concatenate_data_sets thinly: per-statistic files combine in order, shared tracers stored once, covariance block-diagonal (library-enforced all-or-none), metadata union with loud conflicts. update_statistic() is the value-only merge-back for the extract -> conceal -> merge blinding flow: matches each sub point by (data_type, tracers, tags) and overwrites the value, leaving order and covariance untouched. Tests: all four data/mock x concealed/unconcealed quadrants, the escape hatch, stamp validation, merge (points, covariance, metadata conflict, mixed-covariance failure) and update_statistic (values-only, unique-match). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QWF72ofwJh6ekgnCt9Xx6C
…comments - grid tag values: coarse -> "reporting", fine -> "integration" (descriptive, not relative); tag kept because both grids share data type and tracer pair, so the tag is the sole disambiguator. Swept module docstrings and tests. - Module docstring now spells out why insertion order is load-bearing: covariance row/column i refers to the i-th inserted data point. - new_sacc: comment that psf_stars rides in the same tracer list as the NZ tracers only because a Sacc has one flat tracer namespace (bookkeeping, not physics). - source_name/_pair helpers kept (4 and 8 call sites) with one-line justifications of the conventions they centralize. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QWF72ofwJh6ekgnCt9Xx6C
- Factor the theta-tagged insertion loop into _add_theta_series (add_rho, add_tau, add_pure_eb); add_pure_eb zips PURE_TYPES.values() against its signature order, and PURE_KEYS is derived from PURE_TYPES instead of restating it. - Factor the (theta, plus, minus) read pattern into _get_pm (get_xi, get_rho, get_tau). - add_xi hoists the optional-tag None-filtering out of the point loop; extract and merge lose their throwaway mutable dicts; get_cosebis builds its scale-cut tags in one expression. - Tests: shared _add_xi default-ξ builder and _xi_block/_cl_block/ _cosebi_block canonical index-block helpers replace ~60 lines of copy-pasted setup; test_readers_on_mixed_file builds on _multi_statistic_sacc. Behaviour unchanged; 41/41 tests green in the container. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QWF72ofwJh6ekgnCt9Xx6C
cailmdaley
force-pushed
the
feat/sacc-2-sacc-io
branch
from
July 16, 2026 00:39
55cb97d to
4489dd4
Compare
Adversarial review (pass 2) findings: - Sacc.indices returns an EMPTY array (warning only) on an unmatched selection; every reader, extract(), and covariance-block selector now funnels through a shared _indices guard that raises instead — a typo'd tag or non-bitwise-identical float scale cut can no longer propagate empty arrays downstream (assemble_covariance previously died with an opaque IndexError on the same path). - get_cosebis(scale_cut=None) on a file carrying several scale cuts silently concatenated them; now raises and asks for an explicit cut. - update_statistic let two sub points claim the same target point (last-write-wins); now raises. - merge: the library keeps the FIRST input's tracer on a name clash with no equality check; shared tracers are now verified identical (z, nz) across inputs before concatenation. 7 regression tests; 48 total. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzUt7VbtXwr2SCHUdiQTyt
…tion The branch previously carried a stale vendored copy of sacc_io.py / test_sacc_io.py from before PR2's review rounds. Rebuilt directly on feat/sacc-2-sacc-io (8bd3817) so the canonical module is inherited, bringing over the cosmo_val + Snakemake born-as-SACC migration work from the old tip unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzUt7VbtXwr2SCHUdiQTyt
…rename
Sweep the migration code onto PR2's canonical vocabulary and contracts:
- grid='coarse'/'fine' -> 'reporting'/'integration' everywhere (writer
calls, readers, tests), including the internal DAG intermediates:
{version}_xi_coarse* -> _xi_reporting*, _xi_fine -> _xi_integration,
the xi_coarse/xi_fine Snakemake output keys, CANONICAL part names,
cv_xi_reporting_sacc, write_xi_integration_sacc.
- save(s, path) -> save(s, path, type=...): 'data' at every production
writer (the cosmo_val pipeline measures the real UNIONS catalogues;
GLASS mocks do not flow through these writers), 'mock' for synthetic
test fixtures. assemble_sacc propagates its parts' type stamp rather
than hardcoding, so mock parts assemble into a mock analysis file.
- load(path) -> fail-closed load: pipeline-internal readbacks of
freshly written pre-blind data parts pass allow_unblinded=True
(blinding is a downstream Smokescreen step); mock fixtures load freely.
Readers raising on unmatched selections needed no call-site changes:
every get_* reads a statistic guaranteed present in the file just
written or assembled.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WzUt7VbtXwr2SCHUdiQTyt
cailmdaley
force-pushed
the
feat/sacc-4-cosmo-val-sacc
branch
from
July 16, 2026 09:50
149a50c to
72e680f
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
1 similar comment
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Consistency follows tagging semantics: the grid tag declares which binning a set of points lives on, so all same-length theta arrays under one tag value must be bitwise identical (sacc never validates angles, and grids diverging at floating-point level choke CosmoSIS downstream). Different lengths within a tag group pass (scale-cut subsets); grids under different tag values are unconstrained (reporting vs integration differ by design). The rho/tau/pure-EB writers now tag their points grid="reporting" by default (overridable via grid=), so they join xi's consistency group and no untagged group appears in our own files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MN9VazXKHUHQg16kiG7Ufk
…seudo-Cl grid="reporting" The theta consistency guard generalizes to both angular domains: theta and ell points are grouped separately by grid tag value, and within a tag value all same-length grids must be bitwise identical. Two ell series sharing a grid must also carry equal bandpower windows (window ells and weight matrix); series without windows skip that check. add_pseudo_cl now stamps grid="reporting" on every point by default (overridable), joining the merge guard's consistency groups; since sacc's add_ell_cl accepts no extra tags, its per-point insertion (ell + shared window + window_ind) is inlined. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MN9VazXKHUHQg16kiG7Ufk
assemble_covariance now passes sacc.add_covariance a list of per-block matrices instead of a dense zero-filled N×N array. sacc.BaseCovariance.make turns a list into a BlockDiagonalCovariance (one FITS table per block, Σ block² on disk), so cross-blocks are zero and implicit rather than materialized. Validation (contiguous/ascending indices, no gap/overlap, square blocks matching their index span) is unchanged. Every existing consumer already read through the polymorphic .dense property, so only the type assertions needed updating (FullCovariance -> BlockDiagonalCovariance). Added a test that merging two files that already carry a BlockDiagonalCovariance (assembled via assemble_covariance) stays block-diagonal through merge and a save/load round-trip. Updated the module docstring's storage-cost discussion accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pure_eb.py's calculate_pure_eb/plot_pure_eb defaulted nbins_int=100; cosebis.py's calculate_cosebis already defaulted to 1000, and every production config (papers/bmodes, papers/cosmo_val fiducial/pure_eb blocks) already overrides to 1000. This aligns the function-signature default with what every caller actually uses; config plumbing is untouched, so any explicit override still wins. The papers/cosmo_val/config/config.yaml cosebis.nbins_int (currently 2000, production numerics) is deliberately left unchanged — see report. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Writers no longer force components an analysis may not have computed: add_pseudo_cl's BB/EB, add_cosebis's Bn, and add_pure_eb's B/ambiguous blocks now default to None and are simply not written when omitted (add_pure_eb still requires each +/- pair together). Structural arguments (grids, tracer/bin identifiers, the value for a component you ARE adding) remain required with no default. Composite readers (get_pseudo_cl, get_cosebis, get_pure_eb) return None / omit the key for an absent optional component instead of raising, via a new _mean_optional helper; a selection naming a missing component explicitly (s.indices, _mean, extract) still fails loud, per the existing empty-selection guard. merge() and assemble_covariance work unchanged on files with only a subset of components. Documents the optionality contract in the module docstring, and adds partial-file round-trip, merge, and explicit-selection-fails-loud tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fault cosebis.nbins_int was 2000; every other integration-grid entry in this config (pure_eb, the fiducial block) is already 1000, and cosebis.py's own function defaults are 1000. Unify. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…acc-4-cosmo-val-sacc
Fold the integration-grid ξ± into the single terminal {version}.sacc, and
make part loading fail closed on unblinded real data.
Integration ξ± (grid='integration') is no longer its own terminal product.
The xi_highres part is now gathered by rule assemble_sacc into {version}.sacc
as tagged points, next to the reporting ξ± block. It is fiducial-only (the
10k-bin MPI run emits only the fiducial part), so it joins the fiducial
version's terminal file alone. assemble_sacc.py adds xi_integration to
CANONICAL; its own DiagonalCovariance passes straight through.
assemble_sacc.py no longer loads every part with allow_unblinded=True. The
run type (data|mock, from config, default data) gates it: mock runs load
freely, data runs fail closed unless a part carries the concealed=True stamp.
This is the seam for PR #253's blind-at-birth — a concealed data part then
assembles with allow_unblinded=False untouched.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EaL7prmKUHwJQcDyW3LoxD
…creen
The pseudo_cl / pseudo_cl_cov rules and the shared BLINDS list use a 'blind'
wildcard that is the glass-mock multi-catalogue A/B/C variant, not Smokescreen
blinding. Add prominent comments at the BLINDS definition, the wildcard
constraint, and both rules so the two axes are not confused. A full rename is
avoided: 'blind' is baked into on-disk filenames we do not own (external
nz_{version}_{A|B|C}.txt) and into the covariance / inference path builders.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EaL7prmKUHwJQcDyW3LoxD
Resolve two silent semantic conflicts from the base merge (no textual conflict, but the tests broke). - add_cosebis reordered its params to (s, bins, En, scale_cut, Bn=None). cosebis_to_sacc still passed the old positional order; call it by keyword. - assemble_covariance now builds a BlockDiagonalCovariance (one FITS table per block, validated ordering), not a dense FullCovariance. Update the assembler docstrings, the smk comments, and the assembled-file test assertions to the block-diagonal type. The assembled .dense is unchanged (cross-blocks are zero either way); single-part writer covariances stay FullCovariance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EaL7prmKUHwJQcDyW3LoxD
…val-sacc # Conflicts: # src/sp_validation/masks.py # src/sp_validation/tests/test_masks.py # workflow/image_sims/params_im_sim.py
… override machinery The branch had accidentally superseded #266's reproducible-deps model (uv.lock as source of truth, `uv sync --frozen` in the Dockerfile) with an older firecrown/Smokescreen-blinding scheme: uv.lock deleted, dependency resolution done via `uv pip install --overrides uv-overrides.txt`, and a patch script to make pip-installed firecrown importable without NumCosmo. Firecrown has been dropped from the project, so all of that goes. Restores develop's pyproject.toml, uv.lock, Dockerfile, and the deploy-image blinding-stack smoke test wholesale; removes uv-overrides.txt and scripts/patch_firecrown.py. Keeps one genuine PR4-driven change: tightens the sacc constraint to >=2.4,<3, since sacc_io.py (this branch) uses concatenate_data_sets/BlockDiagonalCovariance, both from sacc's 2.x rewrite (develop's lock already resolves sacc to 2.4; this just makes the pyproject floor honest). Drops the unused numexpr addition — no code in the tree imports it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EaL7prmKUHwJQcDyW3LoxD
The rule's inputs are now one ξ± part instead of a TreeCorr .txt plus the integration part, and the three companion figures are declared outputs rather than byproducts landing beside them. The COSEBIs grid (0.9–300 at 1000 bins, patched) is a row in the grid table, so the same xi rule measures it.
The pure-E/B jackknife needed the patched correlation objects, which is the one thing a part cannot carry — and per Cail's ruling jackknife and blinding are a deprecated pair for derived statistics. So the covariance now comes from the Monte Carlo path calculate_pure_eb already had, extracted as pure_eb_covariance_mc: draws from the CosmoCov gaussian covariance on the integration grid, around a theory mean, through the same kernel as the modes. It depends on the covariance model and the grids, never on the measured vector, so it is blind-invariant by construction. cv_pure_eb consumes two parts plus that one covariance file and touches no catalogue. The results dict is now self-describing — the grids travel with the modes (theta, edges, the raw ξ± and their variances, n_eff) instead of a TreeCorr object riding along — so the statistics, plots and npz all work from values. That deletes the FakeGG stub the paper PTE script needed, and turns calculate_eb_statistics into a function of its results alone. calculate_2pcf no longer writes patch results into the .txt dump: a per-patch ξ± realisation is an unblinded data vector, nothing reads one back, and the covariance a consumer needs is the matrix the part carries.
It re-ran plot_pure_eb / plot_cosebis / plot_pseudo_cl in-process to repopulate the in-memory result dicts, which meant the terminal diagnostic reached the catalogue — and rewrote the two consumers' figures from unblinded values on the way past. It now reads what those rules wrote: the pure-E/B PTE matrices and the COSEBIs B-mode PTE from their .npz products, and the pseudo-Cℓ BB spectrum from its part against the NaMaster covariance, both declared inputs. The summary can no longer disagree with the products it summarises. The table itself moves to print_bmode_summary, so the in-memory path (summarize_bmodes, for notebooks) and the file path print the same thing. A test pins the .npz key contract the two rules meet on.
…tags canonical Six findings from the review. One pseudo-Cl producer. The untagged diagnostic part and its rule are gone; the summary, the terminal file and now the figures all read the analysis part with its matching NaMaster covariance, so the three cannot disagree. The figures become a plot-only ingest rule like the other B-mode ones, over a plot_pseudo_cl_spectrum that draws one spectrum for every version — the three near-identical EE/EB/BB blocks were one function all along. The analytic covariance wins, loudly. A part whose analysis covariance is external (ξ± reporting, pseudo-Cl) now always takes the supplied block, which replaces the estimate it was born with rather than losing a race with it, and says so on stdout; a missing injection raises instead of silently keeping the part's own. The reporting part keeps carrying its jackknife — it is a real diagnostic, just not the analysis covariance. Grid tags are canonical. xi_binning stamped raw YAML (maxsep=300) while the measurement wrote float-normalised names (maxsep=300.0), so producer and consumer would have asked for different paths on the first real run. The table coerces once, where it is built. It moved to common.py to be testable at all, and the tests pin the round trip a filename makes. Also: cosebis_from_xi deleted (no callers, and its centre-based cut diverged from the edge-based one in use); the paper PTE script's unread cov_integration and npatch dropped, with the rule docstring corrected to say plainly that nothing there varies by blind; and the header DAG comment rewritten to the SACC-part graph it actually describes.
cailmdaley
marked this pull request as ready for review
August 31, 2026 16:13
Collaborator
Author
|
@sachaguer and @LisaGoh i believe this is ready for review! this was a messy PR since it touches all the snakemake rules and their scripts. it's been hard to keep the scope focused, there is probably some lingering slop to be cut out as we critique. |
…val-sacc Every conflict came from 7dd1984 (snakemake containerization, #309). Resolutions: - pyproject.toml: develop's comment block, minus its reference to run_2pcf_highres.py -- this branch deletes that script, so the PR-side wording ("optional MPI runners") is the one that stays true. - workflow/common.py: develop's REPO_SRC + container-model loading block. The PR-side REPO_ROOT/WORKFLOW_SCRIPTS existed only for the shell form of covariance_process and are unused after the next item; dropped. - workflow/rules/covariance.smk: develop's `script: "../scripts/cosmocov_process.py"`. The dead params.output_stub goes with the shell block (it was never defined in a params: section anyway). - workflow/rules/twopoint.smk: dropped both sides. run_cosmo_val had hardcoded paths and no consumer of its sentinel; develop's parked xi_highres block is dead here because the grid-parameterized `xi` rule covers nbins=10000/npatch=1, and the script it names no longer exists. No multi-node MPI path remains in the tree, so the MPI rationale goes too. - workflow/scripts/cv_cosebis.py, cv_pure_eb.py, cv_summarize_bmodes.py: the PR's imports (no make_cv; numpy/get_cosmo added), minus `from snakemake.script import snakemake`. Same import removed from cv_plot_pseudo_cl.py. - workflow/scripts/cv_pseudo_cl.py: deleted, per the PR's one-producer rule. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`workflow/scripts/run_cosmocov_chain.sh` invoked `cosmo_inference/scripts/cosmocov_process.py`, which was deleted on develop (#236) and relocated as the snakemake-only `workflow/scripts/cosmocov_process.py` (driven by `workflow/rules/covariance.smk`). The chain script's only caller was `papers/bmodes/scripts/run_cov_sweep.sh`, whose own only caller was a human, so both are dead end-to-end and are removed. Tree sweep for the same class of rot, repointing what survives: - README: drop the firecrown/Smokescreen override paragraph. Both files it names (`uv-overrides.txt`, `scripts/patch_firecrown.py`) were removed in f5eb417 when firecrown was dropped, and there is no `blinding` extra left in pyproject. - CLAUDE.md / CONTRIBUTING.md: the single-test example named `tests/test_cosmology.py`, which does not exist; use `tests/test_cosmo_val.py`. - CLAUDE.md: the cosmo_inference section documented a `./pipeline.sh` driver with flags. There is no such script; the pipeline is Snakemake-orchestrated, so the section now carries the invocation from `cosmo_inference/README.md`. - `papers/bmodes/config/ecut_spec.md`: `workflow/config/config.yaml` and `workflow/rules/claims.smk` moved under `papers/bmodes/`; the catalog config is `cosmo_val/cat_config.yaml`, not `code/sp_validation/cosmo_val/…`. - `papers/bmodes/scripts/update_survey_stats.py`: usage docstring still gave its pre-move `workflow/scripts/` path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…pure_eb symlink Same file as common.py's CAT_CONFIG; the symlink is slated for removal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Member
|
Sorry for the late reply! I wasn't able to review every single file as this is a pretty large PR, but the overall logic seems sound. Happy to merge this first to keep the restructuring going! |
LisaGoh
approved these changes
Sep 11, 2026
Member
|
Another aside: I'll put aside the development of the tomographic extension for now, since it's getting rather messy with these two efforts (sacc integration + tomographic extension) running in parallel. Let's fix the sacc io format first, then I'll update the PRs on the tomographic xi_pm and covmat calculations to incorporate this format so effort isn't wasted; what do you think? |
sachaguer
approved these changes
Sep 21, 2026
sachaguer
left a comment
sachaguer
left a comment
Contributor
There was a problem hiding this comment.
sorry for the long overdue review.
I think the spirit of the PR is good and it seems well propagated in the scripts. I could not review everything as it is a very large and dense PR but I think it is good to merge and the ultimate test will be whether we find any bugs using it.
Collaborator
Author
|
thank you both! sounds good Lisa, will merge this ASAP so you can be unblocked on the tomography. sorry that the PR is so dense (i agree), and yes we'll shake the bugs out as we start using it :) i am splitting off some largely unrelated B modes changes (they were motivated by the SACC change, but aren't explicitly SACC related) so that the diff is a bit more focused. |
The generic pseudo_cl rule writes a SACC part, so the paper's C_ell consumers read that instead of the FITS PSEUDO_CELL table: one shared reader (pseudo_cl_io.load_pseudo_cl_data over sacc_io.get_pseudo_cl) for the six scripts, and the claims.smk path helper points at the .sacc part. The bandpower covariance still comes from pseudo_cl_cov's FITS. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeMhD6yCyrgBz5oz87bCtR
This was referenced Sep 25, 2026
…val-sacc Brings in #351 (b_modes arrays API + get_theo_xi unpack) and #352 (dead scripts, doc paths). b_modes.py and test_b_modes.py resolve to develop's side, which is this branch's version plus the unpack fix and its test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeMhD6yCyrgBz5oz87bCtR
generate_cosmocov_ini.py was deleted on develop (#236) and came back with this branch's stale-base rebuild; nothing calls it. The README "Local Installation" section repeats docs/source/installation.rst and is not part of the SACC migration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeMhD6yCyrgBz5oz87bCtR
…val-sacc Brings in #350. The xi rule and run_2pcf keep #350's required, explicit cat_config/output_dir alongside this branch's grid/cov/sacc_out; the save_fits/fits knob stays gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeMhD6yCyrgBz5oz87bCtR
cailmdaley
added a commit
that referenced
this pull request
Sep 28, 2026
Brings in develop's #350 (explicit catalogue config, CV_INIT_KEYS) and the #309 host-snakemake container model. Resolutions: - common.py: the blinding path layout is loaded by file path like container.py (one _load_checkout_module helper), since host snakemake has no sp_validation install and a package import would fail the DAG build. - cv_init_params: CV_INIT_KEYS forwarding plus the derived custody kwargs (run_type, blind_root). - rho_tau_stats / run_rho_tau.py: catalogue config + output dir alongside the run type and blind root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013EGDJZeA8v26eKGAHQqh5j
cailmdaley
added a commit
that referenced
this pull request
Sep 28, 2026
develop's tree is identical to the #251 head merged in the parent commit, so the squash commit carries no further content. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013EGDJZeA8v26eKGAHQqh5j
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
3 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.
Closes #247. Row 4 of PRD #241: cosmo_val data products are born as SACC. Builds on #245 (
sacc_io) and #351 (array-levelb_modes); unrelated cleanup went to #352.Throughout, a part is a self-contained SACC file holding one measurement — one ξ± binning or one pseudo-Cℓ variant, with its tracers, n(z), and its own covariance — written by the rule that computes it. Parts are the pipeline's intermediates; assembly joins them into the one file an analysis consumes.
assemble_saccgathers the parts and writes{version}.sacc. The analysis covariances are injected explicitly — CosmoCov for ξ±, NaMaster for pseudo-Cℓ — replacing any born-with estimate, loudly; a missing injection raises. (OneCovariance takes over the ξ± seam when Add tomographic and 3x2pt covariance realspace measurement #256 lands.)cv_cosebis,cv_pure_eb,cv_plot_pseudo_cl, andcv_summarize_bmodesread SACC parts — no catalogue access, no TreeCorr. COSEBIs covariance propagates linearly from the part's ξ± block; pure-E/B uses the semi-analytic MC covariance (jackknife of derived statistics is retired, as individual patches cannot be blinded with SACC).xirule (reworked along the way): a config-driven grid table (binning,npatch, covariance mode) drives a single rule/driver; each ξ± part is written with its own covariance.Where to review. The diff (36 files) falls into four classes — start with 1, then 3; 2 and 4 follow mechanically:
workflow/scripts/assemble_sacc.py(assembly, covariance injection, the fail-closed gate),src/sp_validation/cosmo_val/sacc_writers.py, the grid table inworkflow/common.py.cv_*scripts and the bmodes paper's C_ℓ readers now read SACC parts instead of recomputing; most of the script churn.workflow/rules/twopoint.smkandcosmo_val.smkrewritten around parts.Tests green in CI (
-m "not slow", 285 passed) incl. new coverage for the grid table, covariance injection/replacement, the writer↔reader.npzcontracts, and the paper's pseudo-Cℓ SACC reader; all cosmo_val and paper B-mode DAGs dry-run clean.— Claude (Fable + Opus) on behalf of Cail.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NmGA86b7YyQn54JFj78ssM