Add tomographic and 3x2pt covariance realspace measurement - #256
Conversation
|
✅ ruff is clean — nothing to fix here. |
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
2 participants
Hello, I've updated the Snakemake workflow to run on OneCovariance for the real space probe, and included tomographic and 3x2pt formats! I've also taken the liberty to clean up the
common.pyscript >.<I've ran the covariance workflow and it works, now we just have to decide what kind of format we want our nz distributions to be (I've assumed a single file with i columns, i being the number of tomographic bins), and how we want to specify the area and sigma_e (I've assumed a comma-separated string).
The output is compatible with the previous Cosmocov-processed file. I'll update the Cosmosis fitting script as a next step in the other issue!