Ngmix v2.0 (CI mirror of #740)#741
Conversation
…to ngmix_update
- bin/ scripts were untracked, causing Docker build to fail - Fix license field to use SPDX string format (MIT) to resolve SetuptoolsDeprecationWarning Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…bugs when exp list is empty
Fix check on zero-valued pixels (most did not pass), now all vignets with at least one != 0 pixel pass.
Syncs the branch with develop to pull in the modernized CI (so the full test suite + example pipeline run on this PR) and resolve drift. Only conflict was uv.lock, regenerated with `uv lock` against the auto-merged pyproject.toml (ngmix 2.4.0 source preserved). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
prepare_ngmix_weights drew the noise image and masked-pixel fill from the unseeded global numpy.random, defeating the per-tile self._rng = RandomState(seed) the module sets for reproducibility. Same tile + same seed gave different shears (observed max|dg1| ~ 3e-5). Thread rng through make_ngmix_observation -> prepare_ngmix_weights and draw via rng.standard_normal. Adds test_metacal_is_reproducible_with_fixed_seed (same seed -> identical noshear g); fails before this fix, passes after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
test_imports.py walks the shapepipe package, but standalone scripts/ files are outside it and never get imported. This extends import-smoke coverage to the validation scripts added with ngmix v2.0. Currently fails on centroid_bias.py, which imports get_guess — a helper removed in the module rewrite; that script's status (restore get_guess / fix import / relocate as legacy) is a developer decision. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
centroid_bias.py is the old-API (ngmix 1.x) bias-validation script: it imports get_guess, a helper the v2.0 rewrite removed (v2.0 seeds fits via ngmix's own guessers). It is only ever run against the test_centroid_bug worktree — the centroid_bug/centroid_fix cases invoke it as SHAPEPIPE_PATH/TEST_SCRIPT with SHAPEPIPE_PATH pointing at that worktree (see run_bias_test.sh), which has its own copy. The duplicate on this branch was never executed and only broke import-smoke. centroid_bias_v2.py supersedes it for ngmix_v2.0. Drops it from the scripts import-smoke list accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cailmdaley
left a comment
cailmdaley
left a comment
There was a problem hiding this comment.
Review of the v2.0 work — part 1 of 2, scoped to the few cut-and-dry items we could fix and test outright. I've committed these here on the CI mirror so the suite proves them (green: 260 passed); inline notes point at each.
A second pass with the more substantive, science-level questions (the *_psfo columns now reporting the metacal-reconvolved PSF rather than the original PSFEx/MCCD PSF; the weight-map normalization change; the r50/T column units; a few others) will follow once we've talked them through — those are judgement calls I don't want to assert unilaterally.
Context for this mirror PR is in the description above. Drafted by Claude, directed by me through the review.
— Claude on behalf of Cail
| gal_guess_flag = True | ||
| wsum = 0 | ||
| for n_e in range(n_epoch): | ||
| noise_img = rng.standard_normal(gal.shape) * sig_noise |
There was a problem hiding this comment.
Reproducibility fix (done in this branch). The module sets self._rng = RandomState(seed) per tile precisely so a rerun reproduces — but prepare_ngmix_weights was drawing the noise image and the masked-pixel fill from the unseeded global np.random.randn. Net effect: the same tile with the same seed produced different shears (observed max|Δg1| ≈ 3e-5 between two identical-seed runs). Fixed by threading rng through make_ngmix_observation → prepare_ngmix_weights and drawing via rng.standard_normal. Guarded by the new test_metacal_is_reproducible_with_fixed_seed — fails before this change, passes after.
— Claude on behalf of Cail
| return np.asarray(res["noshear"]["g"]) | ||
|
|
||
|
|
||
| def test_metacal_is_reproducible_with_fixed_seed(): |
There was a problem hiding this comment.
New property test for the reproducibility fix: same seed → identical noshear g. Red on the pre-fix code (global-RNG leak), green after threading rng. Kept cheap (51 px, 2 epochs) so it runs comfortably in the suite.
|
|
||
|
|
||
| @pytest.mark.parametrize("relpath", VALIDATION_SCRIPTS) | ||
| def test_validation_script_imports_cleanly(relpath): |
There was a problem hiding this comment.
New smoke test + a removal it triggered. tests/unit/test_imports.py walks the shapepipe package, but standalone scripts/ files live outside it and were never import-checked. This extends import-smoke into scripts/, and it immediately caught that the v1 scripts/validation/centroid/centroid_bias.py imported get_guess — a helper the v2.0 rewrite removed (v2.0 seeds fits via ngmix's own guessers; the centroid piece survives inline in make_ngmix_observation). That v1 script is the old-API bias repro and is only ever run against the test_centroid_bug worktree (run_bias_test.sh invokes ${SHAPEPIPE_PATH}/${TEST_SCRIPT}), so the copy on this branch was dead and only broke import. We removed the stranded copy here; centroid_bias_v2.py supersedes it for v2.0. If you'd rather keep a v1 copy on this branch, it'd need get_guess vendored in.
— Claude on behalf of Cail
3 participants
What this is
A same-repo mirror of #740 (@martinkilbinger's "Ngmix v2.0"), pushed to a branch on
CosmoStat/shapepipeso that CI actually runs. All 57 commits are authored by Martin (and carry Lucy Baumont's and Axel Guinot's work) — pushing the branch preserves that authorship unchanged; the only thing this PR adds is a same-repo head so GitHub Actions fires thepull_requestworkflow without the fork-PR approval gate. #740 received no CI runs at all for this reason.Substance is identical to #740 — see that PR for the full description. In short: upgrade ngmix to 2.4.0 and adopt Lucy's new ngmix classes/interface; overhaul the shape-measurement module; centroid-bias fix + validation; v2.0 production-run plumbing.
Going forward, opening PRs directly on
CosmoStat/shapepipe(rather than from a fork) avoids this — fork PRs don't trigger our Docker-image CI without a maintainer approval that wasn't happening.Closes/supersedes #740 once CI is green (leaving that call to Martin).
Review
A detailed review is on its way (read against Martin's checklist plus a science-quality pass). Headline from exercising the new fitter against real ngmix 2.4.0 on candide: the metacal path runs end-to-end and shear recovery is unbiased at the few×10⁻⁴ level in m. Full notes to follow.
— Claude on behalf of Cail