Weekend deep code review — neutral fixes, dead-code removal, §7b PV bound#28
Merged
JasperSeehofer merged 12 commits intoJul 7, 2026
Merged
Conversation
CLAUDE.md + hpc-gpu.md drifted from the code: - Known Bug #1 (unconditional cupy import) struck — guarded since 4894648 in all four GPU modules. - Known Bug #7 (bayesian_inference.py 10% distance error) marked moot — Pipeline A was removed in c1571a2; production uses per-source Cramer-Rao bounds. - Known Bug #8 (WMAP cosmology) rewritten as the documented G11 design choice (OMEGA_M=0.2726 matches Barausse-2012 M1; Planck mismatch is a tracked systematic). - Architecture bullets fixed to name bayesian_statistics.py / simulation_detection_probability.py / posterior_combination.py instead of the deleted Pipeline-A modules; bayesian_inference.py removed from the physics-trigger list. - Parameter-estimation bullet updated (5-point stencil is default since Phase 10). Docs-only; no code touched (campaign-safe). Part of the 2026-07-04 code review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Neutral cluster-script fixes (do NOT restart live jobs — deploy on Monday's git pull): - CLU-03: '|| true' on the grep-miss branches in resubmit_failed.sh and submit_pipeline.sh so the friendly empty-result / diagnostic branches actually run instead of set -euo pipefail aborting first. - CLU-06: orchestrator stop-doc uses the pkill bracket idiom so it can't signal its own ssh wrapper shell. - CLU-07: watcher run-dir glob widened to run_202607*_seed* so seeds whose run dirs are datestamped 07-07+ (orchestrator can delay a seed ~41 h under queue-cap pressure) are still mirrored. Structural cluster items (CLU-01 double-submit, CLU-02 blind drift monitor, CLU-05 --mem, CLU-09 combine afterok) tracked in issue #27 for Monday. Part of the 2026-07-04 code review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ty (issue #16) Isolated the GLADE+ peculiar-velocity VALUE correction on the frozen seed600 event set (live PV-corrected catalogue vs a frame-only variant with the correction removed on the 709k flag2==1 low-z rows). Production --evaluate, 17-value H0 grid, shared 3342-event intersection: 1D (redshift-distance): Δmean(live-noPV) = -0.0142 (-3.3σ), ΔMAP -0.010 2D (with BH mass): Δmean = +0.0012 (+0.2σ) — PV-insensitive, un-railed seed600 is the designed worst case (all-low-z, all 709k corrections). The σ_v marginalization (8568d9f) covers this; no campaign re-scope warranted. Closed #16. Commits the provenance (ANALYSIS.md, README.md, builder, queue, stats), not the 1.7 GB variant CSV or the per-h posteriors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Neutral robustness/provenance fixes from the 2026-07-04 code review (data_simulation and injection_campaign are on the simulation path, not --evaluate; the campaign runs pinned code on the cluster, so these take effect on the next git pull): - SIM-01: cancel the 90s SIGALRM in a try/finally on EVERY path (success, each exception-continue, the quick-gate continue) so no stale alarm fires in inter-iteration code and kills an unattended task. - SIM-02: scope warnings-as-errors with warnings.catch_warnings()/simplefilter so it is restored on every exit path and cannot leak into the host-refill / population sampling between iterations (and stops the global filter list growing unbounded). - SIM-03: per-(stage, exception-class) skip tally, logged at end of data_simulation, so a parameter-correlated CRB-stage drop rate (post-SNR-cut) is measurable. - SIM-04: register a SIGTERM flush handler in injection_campaign so a wall-time cap kill loses at most the events since the last periodic flush, not up to 1999 full-5yr-generator SNRs (mirrors data_simulation's existing handler). - SIM-07: bare 'raise' for the unmatched ValueError instead of raise ValueError(e). - REP-02/03: run_metadata cli_args now serialises the FULL parsed namespace via Arguments.to_dict() (captures normalization_mode, pdet_*, catalog_only, ...), and Arguments.seed is cached so it can't return a fresh random value on repeated access. Full fast suite green (739 passed), mypy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Covers the previously-untested parse_to_reduced_catalog -> read_reduced_galaxy_catalog
round trip on a synthetic raw file: flag {1,3} filter, PV-error NaN->0.0015 floor +
quadrature fold into the redshift error, trailing integer redshift flag, the 8-column
positional reorder, and the PHI_S/THETA_S rename on read. The 8-headerless-column
contract is load-bearing for every downstream reader and drifted once before
(the .stale6col_mar28 / .zhelio_20260702 variants). 5 tests, all green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dist()/cached_dist()/dist_vectorized() accept w_0/w_a but evaluate the ΛCDM-only hypergeometric form, so a wCDM call silently returned the ΛCDM result. Add a shared _reject_unsupported_wcdm() guard that raises NotImplementedError on w_0 != -1 or w_a != 0. Every production call uses the defaults (verified by grep), so this changes no computed value — it only turns a silent wrong answer into a loud one. A real wCDM distance (numerical 1/E quadrature) stays deferred to /physics-change. Also PHY-02: dist_derivative now forwards Omega_m/Omega_de/w_0/w_a to hubble_function (which implements the full CPL E(z)) instead of silently using module defaults; and the module/dist docstrings now say ΛCDM-only for the analytic distance. Full fast suite green (739 passed), mypy clean. GitHub #4. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pipeline A was deleted in c1571a2; these are its last unreachable remnants (verified zero live callers by grep across source, tests, scripts, cluster): - datamodels/galaxy.py (synthetic GalaxyCatalog) + its only consumer test_benchmarks.py (the whole file was just that one slow benchmark). - constants TRUE_HUBBLE_CONSTANT, GALAXY_REDSHIFT_ERROR_COEFFICIENT, FRACTIONAL_LUMINOSITY_ERROR, FRACTIONAL_BLACK_HOLE_MASS_CATALOG_ERROR, LUMINOSITY_DISTANCE_THRESHOLD_GPC — all were galaxy.py-only after c1571a2. - handler.parse_to_reduced_catalog_with_reduced_errors — a no-op (built a local DataFrame and discarded it) with zero callers; drops the now-orphaned dist_to_redshift_error_proagation import + a docstring mention. - bayesian_statistics.single_host_likelihood_grid — a print-and-return-[] debug stub with zero callers. Production single_host_likelihood is untouched. (The dev-only single_host_likelihood_integration_testing cross-check twin is kept.) - scripts/quick_snr_calibration.py — orphaned Phase-12 one-off (SNR_THRESHOLD long settled), referenced by nothing. ruff + mypy clean, full fast suite green (739 passed). Closes the code side of #7. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
All changes are value-neutral for production (dead code or unreachable branches or comments-only): - HPC-06: power_spectral_density now raises ValueError on an unknown TDI channel instead of silently returning an all-zero PSD (which would make inner products inf/nan). Unreachable in production (callers iterate ESA_TDI_CHANNELS = 'AE'). - PHY-07: delete DarkEnergyScenario.de_equation — dead (zero callers) and wrong (divides by w_a instead of the CPL multiply; ZeroDivisionError at the fiducial w_a=0). - cosmological_model.py: fix a stale comment pointing at the removed detection_probability module (now simulation_detection_probability). - PHY-09: correct the M derivative_epsilon comment — the log-uniform midpoint is 10^5.5 ≈ 3e5 M_sun (not the stated ~3e3), and the real rationale is a phase-coherence bound for the oscillatory waveform, not Vallisneri's eps_mach^(1/4)|x|. The epsilon value (1.0) is UNCHANGED. Full fast suite green (739 passed), mypy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sweep Neutral plotting/comment fixes from the code review (figures only; no likelihood or simulation semantics): - PLT-03/04: figure truth-lines and the CRB-derived redshift now use constants.H (TRUTH_H) instead of a hardcoded 0.73 literal, so every 'Injected' marker agrees if the fiducial changes (main.py fig01/fig02, paper_figures axvlines). - PLT-05: the interactive tension-explorer x-axis widened to [0.55, 0.88] so the top of the production 0.60-0.86 h-grid (h=0.86) is no longer rendered off-screen. - PLT-10: catalog_plots plot_glade_completeness docstring says Gpc to match its axis label (was Mpc); dropped the stale 'GalaxyCatalog plotting methods' reference. - Stale-comment sweep: TQ-04 test docstrings say HOST_DRAW_Z_MAX is 1.5 (was 0.5); the bias_investigation test_27 comment no longer points at the deleted datamodels/galaxy.py:64. Full fast suite green (739 passed), ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Verified-findings table (landed vs deferred-to-issue vs verified-correct), the 9 commits on this branch, the campaign risk assessment (eval-node OOM refuted), coverage gaps, and the §7b PV bound summary. Companion to issues #23-#27. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CHANGELOG [Unreleased]: Fixed/Removed/Added entries for the review's neutral changes. TODO: PHYS-6 (wCDM guard) marked done; PHYS-2/PHYS-7 marked moot (datamodels/galaxy.py deleted). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
datamodels/galaxy.py was removed as dead code; the Sphinx build failed on its lingering autodoc directive (ModuleNotFoundError, warnings-as-errors). Remove the automodule block, the architecture module-table row, and mark Limitation 8 resolved-by-removal; drop the galaxy.py file ref in Limitation 1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 7, 2026
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
1 participant
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.
Workflow-orchestrated review of the whole codebase (every
.pyundermaster_thesis_code/,the test suite,
cluster/*), run while the Phase-2 GPU campaign self-drove on the cluster.Full report:
docs/reviews/CODE-REVIEW-20260704.md. Every change here is campaign-neutral— nothing that moves a posterior number or changes what the GPU campaign computes was touched;
those went to GitHub issues instead.
Headline
The physics core is sound. The three densest dimensions (likelihood-core,
selection-function, catalogue-handler) found no critical or major defects. Gray (2020)
likelihood faithfulness,
fixed_quadadequacy (n=100 resolves the completeness feature to~1e-14 even at z_max=3), and p_det h-invariance all verified correct.
The one campaign-risk lead is refuted: depth-1.5 does not blow up eval-node RAM
(mass-bearing rows z≤0.5 = 21.74M vs z≤1.5 = 21.75M, +0.05%; the catalogue is parent-only,
not forked across workers). No Monday OOM fix needed. See the report's "Campaign risk
assessment" (the only residual is
--memunset on sbatch → issue #27).What landed (neutral only)
main.py): SIGALRM cancelled intry/finallyon everypath; warnings-as-errors scoped with
catch_warnings(); per-class skip counters;injection_campaignSIGTERM flush handler (SIM-01/02/03/04/07).main.py,arguments.py): full parsed-namespace inrun_metadata; cachedseed (REP-02/03).
physical_relations.py):NotImplementedErroron non-ΛCDM inputs insteadof a silent wrong result (PHY-01/02, [PHYSICS BUG][MEDIUM] dist() accepts w₀, wₐ but silently uses hardcoded ΛCDM formula #4).
datamodels/galaxy.py, galaxy.py-onlyconstants, a no-op parse method, a debug stub,
DarkEnergyScenario.de_equation(dead +wrong),
scripts/quick_snr_calibration.py(DED-04/05/06/07).(cupy guard already fixed; Pipeline A removed; WMAP→G11 design choice); CHANGELOG + TODO.
constants.H; un-clip the h=0.86 grid point; label/comment fixes.Deferred to issues (semantic — campaign is live)
luminosity- vs rate-weighted f, K-correction.
NaN guard, candidate window vs kernel) — post-harvest
/physics-changebatch.get_redshift_outer_boundsignoressigma_multiplier(2σ requested, 3σ delivered).--seed).--mem).Reconciled stale issues: closed #5/#6 (Pipeline-A moot / WMAP→design-choice), commented #4/#7/#8.
§7b — isolated PV value-correction bound (closes #16)
On the frozen worst-case seed600 set (all-low-z, all 709k GLADE+ PV corrections), removing
the PV value correction shifts the 1D H0 estimate by Δmean = −0.0142 (−3.3σ) and the
production-relevant 2D (with BH mass) channel is PV-insensitive (Δmean +0.0012, +0.2σ,
un-railed 17-value grid). The σ_v marginalization (
8568d9f) covers it; #16 closed.Analysis:
results/pv_correction_test_20260703/ANALYSIS.md.Verification
Full fast suite green (
739 passed), ruff + mypy clean before every commit. 11 atomiccommits; see the report for the per-commit map.
🤖 Generated with Claude Code