Repository navigation
Conversation
|
🤖 autofix pushed |
d235788 to
00a0f6b
Compare
|
thank you Lisa this looks great, please continue to take liberties! review incoming. for the n(z) format, i'm proposing we migrate to SACC (see #241 for motivation, #246 for what this would mean for the OneCovariance interface). but i think it's fine to go ahead with what you've assumed for now so this PR can be merged quickly, and we can do the switch to SACC later. |
There was a problem hiding this comment.
Thanks Lisa — this is a really nice move. The CosmoCov→OneCovariance swap is much cleaner than the old block/cat/process chain, and the COV_BASE_TEMPLATE consolidation plus workflow.source_path fix are genuine improvements to common.py, liberties happily taken. A few things to sort before merge, one of which I think breaks the workflow as-is:
Blocker: build_redshift_path is deleted but still called. workflow/rules/inference.smk line 66 and (worse) line 138 still call build_redshift_path — line 138 is evaluated at parse time, so any snakemake invocation on this branch should die with a NameError, not just covariance targets. Simplest fix: keep build_redshift_path as it was (it's exactly build_redshift_dir(version) + build_redshift_path_lens(version, blind)) alongside the new helpers, or update inference.smk in this PR. (If the covariance run worked for you, I suspect it was against a locally-edited inference.smk — worth double-checking what state that was in.)
Personal-path defaults — let's fix the pattern, not the username. To be fair, the previous defaults were no more neutral: they were Cail's personal tree. Rather than swapping one username for another, let's use this PR to drop personal defaults entirely — make COSMO_VAL / COSMO_INFERENCE / CAT_CONFIG required (env var or config key, loud error if unset), so each of us sets our own and the repo carries nobody's paths. Happy for that to be a small follow-up if you'd rather keep this PR focused. The one that genuinely needs reverting here is COSMOLOGY_PARAMS — inline comment explains why (it's about the generating rule, not whose disk).
Rest is inline — mostly small. On n(z) formats and area/sigma_e specification: as in my comment above, fine to keep your assumed formats for now and revisit with the SACC migration (#241/#246).
— Fable on behalf of Cail
| os.environ.get( | ||
| "COSMO_VAL", "/n17data/cdaley/unions/code/sp_validation/cosmo_val/output" | ||
| "COSMO_VAL", | ||
| "/n23data1/n06data/lgoh/scratch/UNIONS/sp_validation/results/cosmo_val", |
There was a problem hiding this comment.
Rather than these defaults carrying anyone's personal tree (the old values were Cail's, so this is a fix-the-pattern note, not a revert request): suggest requiring COSMO_VAL / COSMO_INFERENCE / CAT_CONFIG explicitly — env var or config key, loud error if unset — so each of us sets our own and the repo carries nobody's paths. Fine as a small follow-up if you'd rather keep this PR focused.
| # Regenerate with: snakemake results/cosmology/planck18.json | ||
| # Resolved relative to the run directory at configure() time. | ||
| COSMOLOGY_PARAMS = "results/cosmology/planck18.json" | ||
| COSMOLOGY_PARAMS = "/n23data1/n06data/lgoh/scratch/UNIONS/sp_validation/results/cosmology/planck18.json" |
There was a problem hiding this comment.
This one does need to stay relative (independent of the defaults question above): the cosmology_params rule generates this file at the relative path results/cosmology/planck18.json in the run directory, so pointing the constant at an absolute path decouples the reader from the generator — the rule would write one file and everything else would read another.
| base_version = re.sub(r"_ecut\d+", "", base_version) | ||
| if "v1.4.11" in base_version: | ||
| base_version = "SP_v1.4.6" | ||
| return f"nz_{base_version}_{blind}_source.txt" # TO DO: check lens and source file format |
There was a problem hiding this comment.
Three copies of the same version-normalization regex block now — worth one _nz_base_version(version) helper that the three functions share.
Also a naming/semantics check to resolve with the TO-DO: in the ini, zlens_file gets build_redshift_path_lens (the existing source-galaxy n(z)) and zclust_file gets ..._source.txt. OneCov's zlens is the lensing source sample and zclust the clustering lens sample — so a file suffixed _source feeding zclust reads inverted. If the intent is _lens.txt for the clustering sample, let's fix the suffix before anyone generates files against this convention.
Minor: build_redshift_dir returns a trailing-slash dir which the rule then passes through os.path.dirname, and the _lens/_source helpers return bare filenames that go through os.path.basename. Both round-trips can go — return dir and filenames and use them directly.
| cov_th = config[base_version]["cov_th"] | ||
| return cov_th["A"], cov_th["n_e"], cov_th["sigma_e"] | ||
|
|
||
| if config["probe_3x2pt"] == "wl": |
There was a problem hiding this comment.
This branches on the global config["probe_3x2pt"] while everything else in the rule branches on the {probe} wildcard — if they disagree (config says 3x2pt, someone requests a wl target, or vice versa) you get an ini with mismatched survey-spec lines. Suggest get_cat_params(version, probe) taking the wildcard, and dropping the global key entirely: then covariance_3x2pt (line 391) can just say probe="3x2pt", which also fixes the slightly bewildering probe_3x2pt: wl in the test config.
| small_k_damping_for1h = damped | ||
|
|
||
| [misc] | ||
| num_cores = 8 |
There was a problem hiding this comment.
Hardcoded 8 here has to match threads: 8 on covariance_onecov by hand (the ini is written by a different rule, so {threads} can't be used directly). Worth a single shared constant (e.g. ONECOV_CORES = 8) used by both, so they can't drift.
| rules.covariance_ini_onecov.output, | ||
| output: | ||
| str(COSMO_INFERENCE / "data/covariance/covariance_{version}_{blind}_{gaussian}_minsep={min_sep}_maxsep={max_sep}_nbins={nbins}{mask_suffix}/cov_tmp_ssss_{block_pm}_cov_Ntheta{nbins}_Ntomo1_{block_i}") | ||
| # Matches `[output settings] file = cov_tmp_onecov.dat` in the ini. |
There was a problem hiding this comment.
Stale comment — the ini's file = is now the full covariance base name, not cov_tmp_onecov.dat.
| ) | ||
| GLASS_MOCK_COV_DIR = config.get("glass_mocks", {}).get( | ||
| "covariance_dir", | ||
| "/automnt/n17data/cdaley/unions/pure_eb/results/covariance/glass_mock_v1.4.6", |
There was a problem hiding this comment.
Making this config-overridable is a nice touch. The default uses the deprecated pure_eb/ compat symlink though — canonical path is now /automnt/n17data/cdaley/unions/analyses/shear_2d/bmodes_2d/results/covariance/glass_mock_v1.4.6.
|
|
||
|
|
||
| def fiducial_covariance_outputs(mask_suffix=""): | ||
| def fiducial_covariance_outputs(mask_suffix="", probe=DEFAULT_PROBE): |
There was a problem hiding this comment.
probe=DEFAULT_PROBE binds the default at function-definition time — it happens to work because configure() runs before the includes, but it's fragile ordering. Safer: probe=None and resolve inside (which is also the pattern covariance_base already uses). Relatedly, DEFAULT_PROBE = "wl" hardcoded inside configure() could just be a module-level constant, or read from config if we ever want a non-wl default.
2c3b003 to
54f5783
Compare
…e/tomo-real-space
Pushed by the lint gate.
|
@cailmdaley I think this is ready to be merged! I updated the PR description, rebased it to merge into develop and proposed an updated workflow (see the latest comment in #375). Once this is merged, we can merge #306 as well! |
Replaces CosmoCov with OneCovariance for the real-space cosmic-shear ξ± covariance, and adds a tomographic variant. This PR only touches the covariance. The tomographic data vector, and putting this covariance into SACC, are in #306.
Brought up to date with
develop(merged, not rebased; T is retired since #402).What changes
workflow/rules/covariance.smkcovariance_iniwrites the OneCovariance.ini;covariance_runruns it and writes a dense ξ± matrix (.dat,np.savetxt): G+NG forng, G only forg.cov_th:sigma_e,n_e,A). The n(z) comes fromshear.redshift_path, and the mask Cℓ fromMASK_CLS_FILES; both are split into directory and file, as OneCovariance expects.covariance,covariance_masked,covariance_unmasked(NG on the reporting grid, G on the integration grid), and the newcovariance_tomo.covariance_ini/covariance_cosmocov/covariance_cat/covariance_process) andworkflow/scripts/cosmocov_process.py.workflow/common.pyCOV_BASE_TEMPLATE, used for rule outputs throughcov_output()and filled in bycovariance_base():covariance_{version}_{gaussian}_minsep=…_maxsep=…_nbins=…{mask_suffix}{tomo_suffix}covariance_base/covariance_dir/covariance_pathtaketomo_suffix.covariance_path()now returns the.datby default, so inference, the glass mocks, cosmo_val (pure-E/B, COSEBIs,assemble_sacc) and the bmodes figures read the OneCovariance matrix without changes of their own.tomo_suffix((_tomo)?) andnpatch.workflow/rules/masks.smk: removedprepare_mask_for_cosmocov.tools.cosmocov_executableis replaced bytools.onecov_executable.Tested
covariance,covariance_tomoand the wholecosmo_valpaper DAG. For the fiducial catalogue, the rendered.inimatches itscov_thand n(z).assemble_sacc, xi-grid and cv-params tests pass.Heads-up for reviewers
COSMO_INFERENCE; on the shared tree the new.iniwould overwrite CosmoCov's..datexists yet, so anything that needs a covariance triggers OneCovariance runs. The non-Gaussian ones are slow._processed.txtstill read the published CosmoCov files, so the paper's numbers are unchanged.matrixoutput (expected: ξ+ then ξ−, by θ; for tomography, bin pairs(1,1), (1,2), …, (n,n)) against an existing_processed.txt.cov_th. With today's non-tomographic fiducial catalogue,covariance_tomoonly renames the single-bin covariance.Out of scope
ggl/ 3x2pt covariances: deferred until there is a lens sample.🤖 Generated with Claude Code