Add Spectral Extraction plugin for Cubeviz #2039

bmorris3 · 2023-02-27T19:53:49Z

Description

I introduced ND array collapses along arbitrary dimensions with propagation of uncertainties, masks, and units in astropy/astropy#14175.

This PR implements a Spectral Extraction plugin for Cubeviz. Users can load a spectral cube, and collapse the entire cube or a spatial subset to a 1D spectrum via the following operations: sum, mean, min, or max. Uncertainties are propagated self-consistently, and logic is built in to flexibly handle masks (for example, do you want collapses along an axis to become masked if any data are masked, versus only if all data are masked? See astropy/astropy#14175 (comment) and related comments for use cases).

Demo

This demo uses a background calibration observation with MIRI. The related target is the bright A3 star HD 163466 (URI: mast:JWST/product/jw01539-c1022_t004_miri_ch1-short_s3d.fits)

JDAT-3113.mov

Discussion points

Naming

In this POC, I've named the plugin "Spectral Extraction", which is an exact name collision with the plugin in Specviz2D:

jdaviz/jdaviz/configs/specviz2d/plugins/spectral_extraction/spectral_extraction.py

Line 33 in e57350c

class SpectralExtraction(PluginTemplateMixin):

That may be ok if we won't have 3D cubes and 2D spectra in the same helper. The only step I took to differentiate is to give the plugin a different name in the tray registry.

Do we want to give these distinct names?

Are pipeline uncertainties smaller than we want for model fitting?

The uncertainties in the ERR extension from the pipeline are quite small. For example, here is a simple comparison between the pipeline uncertainties and the square-root of the data in the SCI extension, which is a proxy for the expected photon noise:

Typical uncertainties are within a factor of three of the photon noise – but the RMS scatter in the spectra show noise that exceeds these errorbars.

Some options for moving forward include:

allow users to edit the uncertainties via jdaviz, while giving users appropriate warnings that their uncertainties are not blessed by the pipeline
never edit uncertainties via jdaviz
- optionally: make a notebook somewhere to document options for scaling uncertainties manually, but intentionally avoid building the functionality into jdaviz

Related upstream PRs

In order to implement the plugin on the jdaviz side, I needed to do several upstream tweaks, outlined here:

Adding support for uncertainty extraction to NDDataArray glue-viz/glue-astronomy#86
- fixed a missing component in the glue Data translator
- Update: this has been released in glue-astronomy 0.7.0
ENH nddata: collapse operations on NDDataArray, improved Masked Quantity support astropy/astropy#14175
- fixed a missing unit propagation in the astropy PR for collapse operations on nddata objects
Bugfix: shuffled arg order in _fit_lines astropy/specutils#1030
- fix enables propagation of errors through the jdaviz Line Analysis plugin
Fix Spectrum1D translator handling of 1D spectra with spectral-only axes glue-viz/glue-astronomy#88
- allows Spectrum1D to be translated to/from glue Data with only spectral coordinates in WCS
Bugfix for bitmasks passed to nddata astropy/astropy#14995
- bugfixes for NDData (Part I)
Bugfix for collapses with NDData without masks, or without units astropy/astropy#15082
- bugfixes for NDData (Part II)

The astropy release calendar indicates that the astropy v5.3 release candidate should be out 2023-04-28, so this PR to jdaviz shouldn't be merged unless we add logic to hide the Spectral Extraction plugin from users with astropy<5.3, until some time in ~May.

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?
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.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

bmorris3 · 2023-02-27T20:08:20Z

CI note: jdaviz/configs/specviz/tests/test_helper.py:test_plot_uncertainties will fail until glue-viz/glue-astronomy#86 is merged.

kecnry

Just a few comments as I was skimming the diff.... its nice to see that this will be quite lightweight on our end if/when the astropy implementation makes it in! 🎉 🤞

kecnry · 2023-02-27T20:38:46Z

jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py

+        # by default we want to use ignore_masked_data=True in nddata:
+        if "ignore_masked_data" not in kwargs:
+            kwargs["ignore_masked_data"] = True
+        # by default we want to propagate uncertainties:
+        if "propagate_uncertainties" not in kwargs:
+            kwargs["propagate_uncertainties"] = True


would these be worth exposing as switches in the UI or are they meant to be advanced use-cases only for the API (in which case maybe they should be explicitly defined in the method, or are you expecting a bunch of other kwargs and need to keep it flexible)?

Suggested change

# by default we want to use ignore_masked_data=True in nddata:

if "ignore_masked_data" not in kwargs:

kwargs["ignore_masked_data"] = True

# by default we want to propagate uncertainties:

if "propagate_uncertainties" not in kwargs:

kwargs["propagate_uncertainties"] = True

# by default we want to use ignore_masked_data=True in nddata:

kwargs.setdefault("ignore_masked_data", True)

# by default we want to propagate uncertainties:

kwargs.setdefault("propagate_uncertainties", True)

This is a good question. The implications of the two options are a bit complex (see astropy/astropy#14175 (comment)) so I'm torn between adding toggles for flexibility and leaving these subtleties in the astropy docs for more advanced users. Happy to hear all opinions!

(un-auto-resolving so we can return to the conversation about these being explicitly listed as kwarg arguments and/or in the UI - I don't have any strong feelings right now, but suspect we'll want to give that some thought when this actually is ready to go in)

kecnry · 2023-02-27T20:39:25Z

jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py

+        # get glue Data objects for the spectral cube and uncertainties
+        [spectral_cube] = self._app.get_data_from_viewer(
+            self._app._jdaviz_helper._default_flux_viewer_reference_name,
+            include_subsets=False
+        ).values()
+        [uncert_cube] = self._app.get_data_from_viewer(
+            self._app._jdaviz_helper._default_uncert_viewer_reference_name,
+            include_subsets=False
+        ).values()


will this become easier with get_data?

Probably/hopefully.

jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py

jdaviz/configs/default/plugins/model_fitting/model_fitting.py

pllim · 2023-02-27T20:53:39Z

Re: CI -- you can theoretically force the CI to install your glue-astronomy PR branch. However, given that your fork does not have proper tags, the glue-astronomy version number will look really old, so you cannot pin glue-astronomy in jdaviz as well. FYI.

pllim · 2023-02-28T15:51:22Z

If you want it to be merged sooner, here is crazy idea. When astropy PR is merged upstream, you can:

have the tests skip unless astropy 5.3.dev or later is detected (so the tests in dev job will pick them up)
disable this plugin unless astropy 5.3.dev is detected
do not include plugin in the viz YAML file (so dev will have to manually insert it in local dev install if they want to use it)

This way, the PR has less chance to go stale.

jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py

kecnry · 2023-02-28T16:08:01Z

jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py

+        [spectral_cube] = self._app.get_data_from_viewer(
+            self._app._jdaviz_helper._default_flux_viewer_reference_name,
+            include_subsets=False
+        ).values()


could this cause issues if the user creates a moment map and sends it to be shown in the flux-viewer (as this plugin would then pull a 2d image instead of the flux cube)?

Yes that would be problematic here. We wouldn't want the user to think they could extract a spectrum from a moment map... should we raise an error if the flux-viewer has >1 Data?

Now reported as a bug 🐱 (although in a slightly different case where the assumption of a single data entry is causing an error)

jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py

bmorris3 · 2023-03-01T18:55:22Z

jdaviz/core/template_mixin.py

@@ -1086,7 +1089,7 @@ def __init__(self, *args, **kwargs):
                                           'spatial_subset_items',
                                           'spatial_subset_selected',
                                           'spatial_subset_selected_has_subregions',
-                                           default_text='No Subset',
+                                           default_text='Entire Cube',


In chatting with @kecnry, we realized we're effectively overriding the default "No Subset" label in both Model Fitting and Line Analysis. Rather than again overriding the default text in another plugin, @kecnry suggested we just change the mixin default directly.

codecov · 2023-03-03T21:50:54Z

Codecov Report

Patch coverage: 77.64% and project coverage change: -0.12% ⚠️

Comparison is base (d33ffcf) 90.72% compared to head (238b6c6) 90.61%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2039      +/-   ##
==========================================
- Coverage   90.72%   90.61%   -0.12%     
==========================================
  Files         157      159       +2     
  Lines       18017    18177     +160     
==========================================
+ Hits        16346    16471     +125     
- Misses       1671     1706      +35

Files Changed	Coverage Δ
...z/configs/default/plugins/line_lists/line_lists.py	`75.21% <ø> (ø)`
jdaviz/configs/specviz/plugins/viewers.py	`83.22% <57.14%> (-1.45%)`	⬇️
...plugins/spectral_extraction/spectral_extraction.py	`68.75% <68.75%> (ø)`
...ctral_extraction/tests/test_spectral_extraction.py	`95.12% <95.12%> (ø)`
jdaviz/app.py	`85.17% <100.00%> (+0.12%)`	⬆️
jdaviz/configs/cubeviz/plugins/__init__.py	`100.00% <100.00%> (ø)`
jdaviz/configs/cubeviz/plugins/parsers.py	`71.49% <100.00%> (ø)`
...aviz/configs/cubeviz/plugins/tests/test_parsers.py	`97.95% <100.00%> (ø)`
...nfigs/default/plugins/plot_options/plot_options.py	`93.35% <100.00%> (ø)`
...igs/specviz/plugins/line_analysis/line_analysis.py	`94.98% <100.00%> (+0.05%)`	⬆️
... and 1 more

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

bmorris3 · 2023-08-15T18:13:09Z

@kecnry Just to clarify, your checkbox above says "pin to 5.3.2," but the PR already disables the Spectral Extraction plugin for astropy<5.3.2. I've restored the astropy pin in this PR to its usual value in jdaviz.

kecnry · 2023-08-15T18:37:39Z

Ok, that works fine too. Maybe add a note in the plugin docs and then create an issue to remove it (and all the test-checks) whenever the pin otherwise gets update to at least 5.3.2?

bmorris3 added 💤 enhancement New feature or request cubeviz discussion Upstream fix required plugin Label for plugins common to multiple configurations labels Feb 27, 2023

bmorris3 requested review from duytnguyendtn, rosteen, javerbukh, pllim, kecnry and haticekaratay as code owners February 27, 2023 19:53

github-actions bot added imviz specviz labels Feb 27, 2023

bmorris3 marked this pull request as draft February 27, 2023 19:57

bmorris3 removed the imviz label Feb 27, 2023

kecnry reviewed Feb 27, 2023

View reviewed changes

kecnry reviewed Feb 28, 2023

View reviewed changes

kecnry reviewed Mar 1, 2023

View reviewed changes

jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py Outdated Show resolved Hide resolved

bmorris3 mentioned this pull request Mar 1, 2023

Adding support for uncertainty extraction to NDDataArray glue-viz/glue-astronomy#86

Merged

bmorris3 force-pushed the spex branch from 95ffdbb to d2c83f3 Compare March 1, 2023 18:52

bmorris3 modified the milestone: 3.4 Mar 1, 2023

bmorris3 commented Mar 1, 2023

View reviewed changes

bmorris3 force-pushed the spex branch 2 times, most recently from 272f40d to 3401ccc Compare March 3, 2023 19:59

bmorris3 added this to the 3.4 milestone Mar 3, 2023

bmorris3 added 21 commits August 15, 2023 14:08

spectral extraction results label bugfix

26c0a53

Better labeling for spectral extraction with subsets

a9f421e

adding changelog entry

10985b3

adding docs entry for SpEx plugin

aa29100

debugging line analysis

92ab0f7

bugfix to prevent nans in some line analysis stats

c5644f5

handle glue-astro SpectralCoordinates in SpEx

ce43bbc

Requiring glue-astro and astropy versions for spectral extraction plugin

191771a

bugfix for ND/1D spectra w/ uncertainties

c5ee73f

bugfix for SpEx name degeneracy

db755e6

pinning new glue-astro release

41a2846

adding test

4ee2b2b

Working towards testing

851dd6b

codestyle fix

3ff86ba

passing along propagated uncertainties through SpEx plugin

2a94d94

Fixing tests, increasing coverage

33dad88

Fixing WCS spectral axis extraction

1f0a9b5

Adding a test that won't be skipped if astropy<5.3.dev

f7fbef5

compatibility fixes for latest jdaviz API, rebase conflict resolution

ea07412

minpin to astropy dev until release of v5.3.2

17b801e

require astropy 5.3.2 for spex plugin

091805c

bmorris3 force-pushed the spex branch from c968990 to 091805c Compare August 15, 2023 18:11

add note to docs about astropy min version for spex

238b6c6

bmorris3 marked this pull request as ready for review August 15, 2023 19:07

bmorris3 merged commit 17a864e into spacetelescope:main Aug 15, 2023
11 of 14 checks passed

This was referenced Feb 19, 2024

BUG: Uncertainty type ignored in Cubeviz spectral extraction #2713

Open

BUG: Mask cube is ignored in Cubeviz spectral extraction #2714

Open

bmorris3 mentioned this pull request Feb 28, 2024

Concept notebook for cube collapse prototype #1969

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Spectral Extraction plugin for Cubeviz #2039

Add Spectral Extraction plugin for Cubeviz #2039

bmorris3 commented Feb 27, 2023 •

edited

Loading

bmorris3 commented Feb 27, 2023

kecnry left a comment

kecnry Feb 27, 2023

bmorris3 Feb 27, 2023

kecnry Feb 27, 2023 •

edited

Loading

kecnry Feb 27, 2023

bmorris3 Feb 27, 2023

pllim commented Feb 27, 2023

pllim commented Feb 28, 2023

kecnry Feb 28, 2023

bmorris3 Mar 15, 2023

kecnry Sep 14, 2023 •

edited

Loading

bmorris3 Mar 1, 2023

codecov bot commented Mar 3, 2023 •

edited

Loading

bmorris3 commented Aug 15, 2023 •

edited

Loading

kecnry commented Aug 15, 2023

Add Spectral Extraction plugin for Cubeviz #2039

Add Spectral Extraction plugin for Cubeviz #2039

Conversation

bmorris3 commented Feb 27, 2023 • edited Loading

Description

Demo

Discussion points

Naming

Are pipeline uncertainties smaller than we want for model fitting?

Related upstream PRs

Change log entry

Checklist for package maintainer(s)

bmorris3 commented Feb 27, 2023

kecnry left a comment

Choose a reason for hiding this comment

kecnry Feb 27, 2023

Choose a reason for hiding this comment

bmorris3 Feb 27, 2023

Choose a reason for hiding this comment

kecnry Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

kecnry Feb 27, 2023

Choose a reason for hiding this comment

bmorris3 Feb 27, 2023

Choose a reason for hiding this comment

pllim commented Feb 27, 2023

pllim commented Feb 28, 2023

kecnry Feb 28, 2023

Choose a reason for hiding this comment

bmorris3 Mar 15, 2023

Choose a reason for hiding this comment

kecnry Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

bmorris3 Mar 1, 2023

Choose a reason for hiding this comment

codecov bot commented Mar 3, 2023 • edited Loading

Codecov Report

bmorris3 commented Aug 15, 2023 • edited Loading

kecnry commented Aug 15, 2023

bmorris3 commented Feb 27, 2023 •

edited

Loading

kecnry Feb 27, 2023 •

edited

Loading

kecnry Sep 14, 2023 •

edited

Loading

codecov bot commented Mar 3, 2023 •

edited

Loading

bmorris3 commented Aug 15, 2023 •

edited

Loading