jdaviz icon indicating copy to clipboard operation
jdaviz copied to clipboard

generalized spectrum parsing, support from Roman ASDF

Open kecnry opened this issue 1 month ago • 3 comments

Description

This pull request generalizes the handling of importing spectra in the new loaders infrastructure to share logic for any dimensionality and parser/input, with the goal to eventually add support for ASDF/Roman.

In doing so, even when going through the specutils parser, all spectrum importers still expose extension dropdowns (for specutils.Spectrum inputs, this just exposes None - when applicable - or specutils.Spectrum as options).

Supersedes #3823

Open questions or potential follow-ups:

  • add extension checkmark support from #3813
  • support extension multiselect (perhaps as part of the merge of SpectrumList > 1D Spectrum)
  • data-label to react to extension (probably only makes sense for Roman or maybe cases where there are multiple choices in the extension select)
  • 2D spectrum: give the user the option to import the trace during import?
  • 2D spectrum: use wavelength min/max to populate the rectified wavelength axis?
  • 2D spectrum: should we load spectrum, spectrum_decon, or do we need to allow the user to choose at load-time?
  • 2D spectrum: is it safe to assume the shape or do we need to detect or allow the user to specify the spectral axis?

TODO:

  • [x] make Image importer invalid for 1d spectral inputs: https://github.com/spacetelescope/jdaviz/pull/3864#pullrequestreview-3424168869

Change log entry

  • [ ] Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts, list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • [ ] Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • [ ] Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • [ ] Do the proposed changes follow the STScI Style Guides?
  • [ ] Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • [ ] Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • [ ] If new remote data is added that uses MAST, is the URI added to the cache-download.yml workflow?
  • [ ] Did the CI pass? If not, are the failures related?
  • [ ] Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
  • [ ] After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

kecnry avatar Oct 31 '25 14:10 kecnry

Codecov Report

:x: Patch coverage is 89.60000% with 26 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 88.16%. Comparing base (964f372) to head (6552155). :warning: Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
jdaviz/core/loaders/importers/spectrum_common.py 90.50% 19 Missing :warning:
...iz/core/loaders/importers/spectrum1d/spectrum1d.py 81.81% 4 Missing :warning:
jdaviz/core/loaders/importers/image/image.py 78.57% 3 Missing :warning:

:x: Your patch status has failed because the patch coverage (89.60%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3864      +/-   ##
==========================================
+ Coverage   88.14%   88.16%   +0.01%     
==========================================
  Files         194      195       +1     
  Lines       26627    26783     +156     
==========================================
+ Hits        23471    23612     +141     
- Misses       3156     3171      +15     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Nov 04 '25 14:11 codecov[bot]

There is one remaining test failure related to #3872 that I'll continue looking into, but I think reviews can start in the meantime

kecnry avatar Nov 05 '25 17:11 kecnry

Although Image is a valid Importer option for the 1D roman file, it fails to load as an image... and if this is supposed to work, it's worth adding a test once that's fixed.

Thanks for the reminder - I had noticed this while demoing but forgot to write it down, I just need to find the condition to make Image not valid.

kecnry avatar Nov 05 '25 21:11 kecnry

Although Image is a valid Importer option for the 1D roman file, it fails to load as an image... and if this is supposed to work, it's worth adding a test once that's fixed.

Thanks for the reminder - I had noticed this while demoing but forgot to write it down, I just need to find the condition to make Image not valid.

The option to load the asdf files as images is still present in the UI and fails for both the 1 and 2D Roman files (ValueError: Data labels ['wfi_spec_decontaminated_...'] not able to be loaded into 'Image'. Must be one of: [])

MatthewPortman avatar Nov 11 '25 21:11 MatthewPortman

A couple things I noticed testing this:

  1. Loading the 1D spectrum and then the 2D spectrum from the test leads to an error ( UnitConversionError: 'pix' and 'nm' (length) are not convertible), since the extracted spectrum has spectral units of pixels and the one from the file has units of nanometers. I don't think this is due to this PR; if it's not we should make a follow up ticket to handle this more gracefully (the viewer with the 2D spectrum should still show up at least). Currently the whole data load fails.

  2. After running into the error above, I tried moving the 1D viewer to see if the 2D was created and hidden under it, and the viewer just disappeared when I tried to drag and drop it:

https://github.com/user-attachments/assets/9db7938d-4da9-4207-9b34-42ba531f1a90

EDIT: problem 1 above is due to _link_new_data_by_component_type trying to LinkSameWithUnits and happens even if the extracted data is set to go into a new viewer.

rosteen avatar Nov 13 '25 17:11 rosteen

I think those are just general issues with deconfigged that need fixing, but not directly related to Roman loaders, so let's create follow-up tickets or add those to the scope of the tickets we're working on next week to test deconfigged further.

kecnry avatar Nov 13 '25 18:11 kecnry