Skip to content

TiltCorrection and TiltSolution improvements - #320

Open
hpparvi wants to merge 4 commits into
astropy:mainfrom
hpparvi:v110_tilt_solution_resampling_fix
Open

hpparvi wants to merge 4 commits into
astropy:mainfrom
hpparvi:v110_tilt_solution_resampling_fix

Conversation

@hpparvi

@hpparvi hpparvi commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

This PR

  • improves the TiltSolution.resample method by propagating uncertainties (if available), propagating the image mask, and copying the NDData metadata to the resampled frame,

  • improves the stability of the TiltCorrection.find_arc_lines method, and

  • adds a baseline subtraction option to line_matching.find_arc_lines (also related to arc line search stability).

AI and LLM disclaimer: same as in #319.

@codecov

codecov Bot commented Sep 5, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.40260% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.65%. Comparing base (4e330bc) to head (41f85a5).

Files with missing lines Patch % Lines
specreduce/line_matching.py 94.28% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #320      +/-   ##
==========================================
+ Coverage   92.52%   92.65%   +0.12%     
==========================================
  Files          18       18              
  Lines        2341     2409      +68     
==========================================
+ Hits         2166     2232      +66     
- Misses        175      177       +2     

☔ 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.

@hpparvi hpparvi added this to the v1.10 milestone Sep 17, 2026
hpparvi and others added 4 commits September 22, 2026 16:17
…nd metadata. The uncertainty is now propagated through the resampling and returned in the same uncertainty class as the input, the mask flags every bin that received a contribution from a masked pixel, and the metadata is copied to the resampled NDData.
…_arc_lines` gained `subtract_baseline` and `baseline_window` options that estimate the baseline flux with a sigma-clipped median (globally, or in running windows with linear edge extrapolation) and remove it before the threshold-based line detection. `TiltCorrection.find_arc_lines` exposes the same options with subtraction on by default, so it no longer requires background-subtracted arc frames to detect any lines.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Pfq3Gc7Boe9TKDs7DVE64
@hpparvi
hpparvi force-pushed the v110_tilt_solution_resampling_fix branch from 15c8b36 to 41f85a5 Compare September 23, 2026 08:59
@hpparvi hpparvi changed the title TiltSolution.resample fixes TiltCorrection and TiltSolution improvements Sep 23, 2026
Comment thread CHANGES.rst
the baseline flux of the spectrum with a sigma-clipped median (globally, or in running
windows) and remove it before the line detection. Baseline subtraction is on by default
in ``TiltCorrection.find_arc_lines``, which previously required background-subtracted
arc frames to detect any lines. [#XXX]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can fill this placeholder with this PR's number.

@tepickering tepickering left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall. The changes that should be made before merging are to fix the NaN poisoning, pass the mask to _estimate_baseline, and decide where the baseline subtraction default is set. I also noted the inconsistency between this and #319 w.r.t. empty masks being False vs None. Either is fine, but consistency is good. Better per-row normalization and handling non-square images with disp_axis=0 are probably worthy of their own issues.

fwhm: float | u.Quantity = 5.0 * u.pix,
window: float = 3.0,
noise_factor: float = 5.0,
subtract_baseline: bool = True,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Defaulting to True here changes more than TiltCorrection: WavelengthCalibration1D.find_lines calls this function without passing the flag (wavecal1d.py:322), so wavelength-calibration line finding now silently subtracts a baseline too. The changelog only says baseline subtraction is "on by default in TiltCorrection.find_arc_lines".

Two options, either is fine with me:

  1. Default to False here and pass subtract_baseline=True from TiltCorrection.find_arc_lines. This keeps the existing wavecal1d behaviour unchanged.
  2. Keep True, but add a changelog note about the change to WavelengthCalibration1D.find_lines, and expose subtract_baseline/baseline_window there so users can switch it off.

# Calculate a normalization factor 'n' for flux conservation. This factor accounts for the
# change in pixel size due to the distortion, and ensures that the total flux in each row
# is conserved after tilt-correction
n = flux.sum(1) / (dtdx * flux).sum(1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The per-row normalization isn't NaN-safe, so one bad pixel poisons its whole output row. A quick check with a 20×30 frame and a single NaN pixel:

  • mask_treatment="apply": all 30 bins in that row are NaN in both flux and uncertainty, but only 2 bins are masked.
  • mask_treatment="nan_fill": the whole row is NaN and nothing is masked.
  • An all-zero row (e.g. after zero_fill, or padding) gives 0/0, so that row is NaN as well.

This was already the case before the PR, but the new docstring promise that the mask marks every bin that received a contribution from a masked pixel makes it misleading now. Computing n from good pixels only would fix most of it, something like:

good = ~mask & np.isfinite(flux)
fgood = np.where(good, flux, 0.0)
den = (dtdx * fgood).sum(1)
n = np.divide(fgood.sum(1), den, out=np.ones(ny), where=den != 0)

There's a smaller version of the same problem in the multi-pixel branch: flux[~m, imin:imax] * k multiplies NaN by the zero weights outside a row's range, giving nan * 0 = nan, so a neighbouring bin can turn NaN without being flagged (the mask only counts w > 0). Using fgood there as well, or np.where(k > 0, flux, 0), would avoid it.


detected_lines = find_lines_threshold(spectrum, noise_factor=noise_factor)
if subtract_baseline:
baseline = _estimate_baseline(spectrum.flux.value, window=baseline_window)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The baseline estimate ignores spectrum.mask. Masked but finite pixels (saturated lines, bad columns) still go into the sigma-clipped median. Could we pass the mask through? sigma_clipped_stats accepts one directly:

baseline = _estimate_baseline(
    spectrum.flux.value, window=baseline_window, mask=spectrum.mask
)

and in _estimate_baseline, use sigma_clipped_stats(flux, mask=mask, ...) for the global median and mask=None if mask is None else mask[lo:hi] for each chunk.

Comment on lines +408 to +410
return NDData(
resampled_flux * im.unit, uncertainty=uncertainty, mask=resampled_mask, meta=meta
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small consistency point: here the output always carries a mask array, all False when the input had none, whereas WavelengthSolution1D.resample() in #319 returns mask=None when the input has no mask. Either convention is fine, but it would be nice if the two resample methods matched. Here the natural place to check is getattr(flux, "mask", None), read before parse_image, the same way meta and has_uncertainty are.

Comment on lines +306 to 313
Notes
-----
Each output bin is a linear combination of detector pixels,
``F = n * sum_j k_j f_j`` with ``k_j`` the fractional pixel overlap times the
Jacobian of the transformation and ``n`` the per-row flux-conservation factor.
The variance is propagated as ``Var = n**2 * sum_j k_j**2 var_j``, assuming
independent pixel noise and treating ``n`` and the Jacobian as deterministic.
"""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One caveat worth adding here: n is computed from the data itself (flux.sum(1) / (dtdx * flux).sum(1)), so it isn't deterministic in practice. On sky- or background-subtracted frames, where a row sums to roughly zero, n becomes unstable, and since the propagated variance is scaled by n**2, the uncertainties go with it. Maybe a sentence like:

Suggested change
Notes
-----
Each output bin is a linear combination of detector pixels,
``F = n * sum_j k_j f_j`` with ``k_j`` the fractional pixel overlap times the
Jacobian of the transformation and ``n`` the per-row flux-conservation factor.
The variance is propagated as ``Var = n**2 * sum_j k_j**2 var_j``, assuming
independent pixel noise and treating ``n`` and the Jacobian as deterministic.
"""
The variance is propagated as ``Var = n**2 * sum_j k_j**2 var_j``, assuming
independent pixel noise and treating ``n`` and the Jacobian as deterministic. Since
``n`` is estimated from the flux of each row, it is unreliable for rows whose total
flux is close to zero, such as in background-subtracted frames.

A more robust per-row normalization could be a follow-up issue rather than part of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants