Skip to content

Address #295 review: epsilon_tv, public rho_psi helper, gradient-test consolidation#310

Merged
rohandahale merged 9 commits into
dev-backendfrom
refactor/jax-review-followups
Jun 22, 2026
Merged

Address #295 review: epsilon_tv, public rho_psi helper, gradient-test consolidation#310
rohandahale merged 9 commits into
dev-backendfrom
refactor/jax-review-followups

Conversation

@rohandahale

@rohandahale rohandahale commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #295 three review notes

Closes the three follow-up notes Andrew left when approving #295, in one PR off dev-backend.

1. epsilon_tv in the spectral-index TV

multifreq_imager_utils.py: reg_tv_spec / reggrad_tv_spec now read epsilon = kwargs.get('epsilon_tv', 0.) like every other TV regularizer, and the hardcoded module-global EPSILON = 1.e-12 is gone. This makes the spectral TV consistent with reg_tv/reg_ptv/reg_vtv and lets a caller tune it through the existing epsilon_tv plumbing (RegParams**reg_params._asdict()). Behavior note: the default smoothing flips 1e-12 → 0 to match the others.

2. _rho_psi_safe → public rho_psi_from_mfrac_vfrac

pol_imager_utils.py: the jax-NaN-safe ρ/ψ helper (double-where guards at zero polarization) is now public, per Andrew's "could have utility outside the pol transforms." Pure rename + docstring; two call sites (mcv, vcv) updated.

3. Gradient-test consolidation

The finite-difference gradient tests were scattered across five files with non-uniform methodology (forward vs central differences, per-pixel-relative vs scale-aware metrics, sampled vs full-grid), two real coverage holes, and a couple of tolerances loose enough to pass a real bug. This rebuilds them around one canonical FD suite and closes the gaps.

tests/test_gradients.py — the canonical numpy FD suite. Six parallel sections, one shared harness (central differences + max-fractional-error relative to the per-slot gradient scale + a non-vacuousness guard):

Section Coverage
S1 chi² Stokes-I vis, amp, logamp, bs, cphase, cphase_diag, camp, logcamp, logcamp_diag × {direct, nfft}
S2 chi² pol pvis, m, vvis × {direct, nfft} (all four physical slots)
S3 reg Stokes-I every name in REGULARIZERS (16, incl. l1, flux, flux_mf)
S4 reg pol every name in REGULARIZERS_POL (8, all four slots, ptv un-xfailed)
S5 reg spectral every name in REGULARIZERS_SPECTRAL (12)
S6 transforms mcv, vcv, polcv (dense objective so every solver slot is exercised)

Coverage holes closed. logamp's gradient had no finite-difference check anywhere in the repo; l1/flux/flux_mf had no numpy-suite FD check (the old tests iterated iu.REGULARIZERS = 12, but the dispatcher uses REGULARIZERS = 16). All now FD-validated. (logamp is log|V|, singular at a visibility null, so its section uses a compact source + debias=False to keep |V| well-conditioned — the gradient itself is correct, verified analytically.)

Tolerances tightened. test_chisquared.py's GRAD_MAX_TOL_NFFT = 10.0 (direct↔nfft gradients actually agree to ~3e-5) is replaced by a 1e-2 bound; because the direct path is now FD-validated here, that bound effectively pins nfft gradient correctness. The pol-TV regularizer max tolerance of 2.0 is gone with the sampled tests it guarded.

Redundancy removed / relabeled. The sampled forward-difference parity classes in test_regularizers.py and the pol FD in test_chisquared.py are subsumed by the canonical suite and deleted (value tests, the TV boundary backstop, cm, and the epsilon-singularity test stay). The reg-only jax tests moved from test_objective_jax.py to test_regularizers_jax.py. TestComputeRegularizerTerm is relabeled as a routing/forwarding check (backend and legacy share the same leaf, so it is not a correctness test).

Rigor check (bug injection). To confirm the suite actually catches bugs, three deliberate faults were injected and each was caught at a fractional error of 0.5–1.9 (≫ the 1e-3 tolerance), with unaffected siblings staying green; all reverted:

  • dropping the mcv_grad ψ-cross-term (the historical bug class) → S6 mcv fails;
  • a factor-2 in chisqgrad_logamp → S1 logamp fails;
  • a factor-2 in reggrad_tv_spec → S5 tv_* fail.

@rohandahale rohandahale changed the title Address #295 review notes: epsilon_tv, public rho_psi helper, gradient-test consolidation Address #295 review: epsilon_tv, public rho_psi helper, gradient-test consolidation Jun 22, 2026
@rohandahale rohandahale requested a review from achael June 22, 2026 06:22
@rohandahale rohandahale self-assigned this Jun 22, 2026
@rohandahale rohandahale added this to the 2.0 milestone Jun 22, 2026
@rohandahale rohandahale marked this pull request as ready for review June 22, 2026 06:22
@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.43%. Comparing base (05a4529) to head (065c105).

Additional details and impacted files
@@             Coverage Diff              @@
##           dev-backend     #310   +/-   ##
============================================
  Coverage        47.43%   47.43%           
============================================
  Files               55       55           
  Lines            26978    26979    +1     
  Branches          4599     4599           
============================================
+ Hits             12797    12798    +1     
  Misses           12689    12689           
  Partials          1492     1492           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rohandahale rohandahale marked this pull request as draft June 22, 2026 06:30
@rohandahale rohandahale marked this pull request as ready for review June 22, 2026 06:30

@achael achael left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! the test consolidation is quite helpful. no comments

@rohandahale

Copy link
Copy Markdown
Collaborator Author

@achael Great! Merging this branch.

@rohandahale rohandahale merged commit 20e604e into dev-backend Jun 22, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants