Skip to content

Rectify now accepts str and HDUList + stop mangling METISLMSSPectralTrace params dict - #985

Open
astronomyk wants to merge 3 commits into
mainfrom
fix/spectral-trace-robustness
Open

Rectify now accepts str and HDUList + stop mangling METISLMSSPectralTrace params dict#985
astronomyk wants to merge 3 commits into
mainfrom
fix/spectral-trace-robustness

Conversation

@astronomyk

@astronomyk astronomyk commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Human-readbale summary:

Bug 1: rectify_cube() and traces() ignore their own checks for filename or HDUList at the top of the functions.
Bug 2: map_spectra_to_focal_plane continues even when chucking an error on a XiLamImage call. Fails again two lines later (line 235) on xilam_wcs = xilam.wcs
Bug 3: Copy of params dict, to avoid altering the original contents of the dict being passed to the function

For more information, see the toaster's explanation below the line. But I think the code changes are more informative anyway


Three independent, few-line fixes, kept as separate commits for review:

1. rectify_traces / rectify_cube crash when given a filename

Both methods document accepting "str or fits.HDUList" and open the file into inhdul — but then passed the original argument on to make_image_interpolations() and SpectralTrace.rectify(). With a filename this raised AttributeError: 'str' object has no attribute 'header'; with an in-memory HDUList it happened to work, masking the bug. Now inhdul is used consistently after opening.

2. map_spectra_to_focal_plane turned a handled error into a NameError

A ValueError raised while building the XiLamImage was caught and logged, but execution then continued with the unbound local variable:

NameError: name 'xilam' is not defined

The method now returns None — the same "no contribution from this trace" contract as the footprint-outside-FoV and empty-footprint paths, which all callers already handle.

3. MetisLMSSpectralTrace wrote into the trace list's meta dict

MetisLMSSpectralTraceList.make_spectral_traces passes params=self.meta by reference to each of the 28 slice traces; each trace then updated that shared dict with its slice-specific values (slice, aperture_id, plus construction kwargs). After building all traces the list's meta carried whichever slice came last. The constructor now copies the dict at the boundary.

Testing

One regression test per fix:

  • TestRectifyTracesInput::test_rectify_traces_opens_filename_before_use
  • TestMapSpectraErrorPath::test_returns_none_when_xilamimage_fails
  • TestMetisLMSSpectralTraceInit::test_does_not_mutate_caller_params

Notebooks demonstrating each fix on the METIS IRDB (including the full LSS observe→readout→rectify-from-file chain for fix 1) are attached below.

@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.09%. Comparing base (beb191d) to head (ae14246).

Files with missing lines Patch % Lines
scopesim/effects/metis_lms_trace_list.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #985      +/-   ##
==========================================
+ Coverage   76.46%   77.09%   +0.62%     
==========================================
  Files          69       69              
  Lines        9025     9027       +2     
==========================================
+ Hits         6901     6959      +58     
+ Misses       2124     2068      -56     

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

@astronomyk astronomyk changed the title Three small robustness fixes in the spectral trace machinery Rectify now accepts str and HDUList + stop mangling 'METISLMSSPectralTrace' params dict Aug 24, 2026
@astronomyk astronomyk changed the title Rectify now accepts str and HDUList + stop mangling 'METISLMSSPectralTrace' params dict Rectify now accepts str and HDUList + stop mangling METISLMSSPectralTrace params dict Aug 24, 2026
@astronomyk
astronomyk marked this pull request as ready for review August 24, 2026 13:44
@astronomyk
astronomyk requested a review from oczoske August 24, 2026 13:44
@teutoburg teutoburg moved this to 👀 Awaiting Review in ScopeSim-development Aug 24, 2026
@teutoburg teutoburg added enhancement PR adding or improving a feature (use "Feature" type for issues (requests), not this label) spectroscopy Related to spectroscopic modes labels Aug 24, 2026
@teutoburg

Copy link
Copy Markdown
Contributor

What practical issues did this cause?

self._xilamimg = xilam # ..todo: remove or make available with a debug flag?
except ValueError:
logger.warning(" ---> %s gave ValueError", self.trace_id)
return None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this was a debugging thing. The idea was to see all cases where a trace gave a ValueError. If we want to finish on the first one, we can simply remove the try...except.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'd be happy with simply removing the try/except, as that forces it to fail loudly... right?

Both methods accept a filename or an HDUList and open the file into
inhdul, but then handed the original argument to
make_image_interpolations and SpectralTrace.rectify. With a filename
this crashed (a str cannot be indexed as an HDUList); with an in-memory
HDUList it happened to work, masking the bug.

Use inhdul consistently after opening.

Adds a regression test that feeds a filename through rectify_traces and
asserts the traces receive the opened HDUList.
A ValueError raised while building the XiLamImage was caught and
logged, but execution then continued with the unbound local variable,
turning the handled error into a NameError. Return None instead, which
callers already handle (the same contract as the footprint-outside-FoV
and empty-footprint paths).

Adds a unit test that simulates the failure.
MetisLMSSpectralTraceList passes params=self.meta by reference; each
trace then updated that dict with its slice-specific values (slice,
aperture_id, plus any construction kwargs). After building 28 traces
the list's own meta carried the values of whichever slice was built
last, and traces could see each other's entries.

Copy the dict at the constructor boundary.

Adds a regression test asserting the caller's dict is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement PR adding or improving a feature (use "Feature" type for issues (requests), not this label) spectroscopy Related to spectroscopic modes

Projects

Status: 👀 Awaiting Review

Development

Successfully merging this pull request may close these issues.

3 participants