jdaviz
jdaviz copied to clipboard
generalized spectrum parsing, support from Roman ASDF
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 toCHANGES.rstbefore merge. If no, maintainer should add ano-changelog-entry-neededlabel.
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
triviallabel. - [ ] 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.ymlworkflow? - [ ] 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)?
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.
: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.
There is one remaining test failure related to #3872 that I'll continue looking into, but I think reviews can start in the meantime
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.
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: [])
A couple things I noticed testing this:
-
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. -
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.
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.