Add ability to export subsets

[cshanahan1]

Blocked by

• Export plugin: table support #2755

Description

This PR adds the ability to export non-composite spatial subsets through the export plugin.
(JDAT 4239)

(I branched off of #2755, which should be merged first.)

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. Bugfix milestone also needs an accompanying backport label.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[pllim]

Since #2755 still in draft and this depends on that one, I have turned this into draft. Thanks for your understanding!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.80%. Comparing base (f1ba4a6) to head (86799ee).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2760      +/-   ##
==========================================
+ Coverage   88.72%   88.80%   +0.08%     
==========================================
  Files         110      110              
  Lines       16353    16394      +41     
==========================================
+ Hits        14509    14559      +50     
+ Misses       1844     1835       -9     

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

[bmorris3]

I opened CubevizExample.ipynb, loaded the file, created a spatial subset, and opened Export:

When I toggle from any viewer to any other, I get the following traceback:

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/ipywidgets/widgets/widget.py:773, in Widget._handle_msg(self, msg)
    771         if 'buffer_paths' in data:
    772             _put_buffers(state, data['buffer_paths'], msg['buffers'])
--> 773         self.set_state(state)
    775 # Handle a state request.
    776 elif method == 'request_state':

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/ipywidgets/widgets/widget.py:650, in Widget.set_state(self, sync_data)
    645         self._send(msg, buffers=echo_buffers)
    647 # The order of these context managers is important. Properties must
    648 # be locked when the hold_trait_notification context manager is
    649 # released and notifications are fired.
--> 650 with self._lock_property(**sync_data), self.hold_trait_notifications():
    651     for name in sync_data:
    652         if name in self.keys:

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/contextlib.py:144, in _GeneratorContextManager.__exit__(self, typ, value, traceback)
    142 if typ is None:
    143     try:
--> 144         next(self.gen)
    145     except StopIteration:
    146         return False

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/traitlets/traitlets.py:1510, in HasTraits.hold_trait_notifications(self)
   1508 for changes in cache.values():
   1509     for change in changes:
-> 1510         self.notify_change(change)

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/ipywidgets/widgets/widget.py:701, in Widget.notify_change(self, change)
    698     if name in self.keys and self._should_send_property(name, getattr(self, name)):
    699         # Send new state to front-end
    700         self.send_state(key=name)
--> 701 super().notify_change(change)

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/traitlets/traitlets.py:1525, in HasTraits.notify_change(self, change)
   1523 def notify_change(self, change: Bunch) -> None:
   1524     """Notify observers of a change event"""
-> 1525     return self._notify_observers(change)

File ~/miniconda3/envs/jdaviz-only-7/lib/python3.11/site-packages/traitlets/traitlets.py:1568, in HasTraits._notify_observers(self, event)
   1565 elif isinstance(c, EventHandler) and c.name is not None:
   1566     c = getattr(self, c.name)
-> 1568 c(event)

File ~/git/jdaviz/jdaviz/configs/default/plugins/export/export.py:189, in Export._sync_singleselect(self, event)
    187     setattr(self, attr, '')
    188 if attr == 'subset_selected':
--> 189     self._set_subset_not_supported_msg()

File ~/git/jdaviz/jdaviz/configs/default/plugins/export/export.py:199, in Export._set_subset_not_supported_msg(self, msg)
    197 if self.subset.selected is not None:
    198     subset = self.app.get_subsets(self.subset.selected)
--> 199     if self.app._is_subset_spectral(subset[0]):
    200         self.subset_invalid_msg = 'Export for spectral subsets not supported.'
    201     elif len(subset) > 1:

KeyError: 0

Any idea what's up there?

[cshanahan1]

@bmorris3 did my recent commit fix the error you were getting?

[bmorris3]

It seems like any viewer is de-selected when I select a subset:

Try testing on your branch?

[kecnry]

@bmorris3 - that is by design, you currently can select any single item across all the sections for export.

[bmorris3]

@kecnry @cshanahan1 I assumed something was wrong because the de-select happens and then nothing is exported. So I suppose only the latter part is the bug. 🙈

Could we have a message in the subset section that says "Selected subsets will be exported from all viewers".

[kecnry]

Could we have a message in the subset section that says "Selected subsets will be exported from all viewers".

It's a single subset - defined the same way in all viewers, right? Is the confusion whether you'll get the spatial subset itself or the spectrum collapsed/extracted from that subset? (If so, that should go away when we stop sending spatial subsets through glue auto-collapse and instead generate data entries that will be listed as data entries here eventually)

[bmorris3]

Correct – if I export a spatial subset, it appears in three viewers because it could have at least three outputs (flux-viewer output, uncer-viewer output, spectrum-viewer output). It might be sensible to return specific outputs, but this UI doesn't explain itself to me.

[kecnry]

This will export the subset definition itself (and is one PR in a larger effort that was already planned/designed, so some of this is probably out of scope and can be dealt with in the final design review). Would just changing the heading from "Subset" to "Subset Definition" (or similar) help?

[bmorris3]

(I promise I'm not trying to be annoying) Even I'm not sure what "subset definition" means in this context if I'm exporting a FITS file – is it a binary mask with metadata that we can pass into glue? Exporting a region is clearer to me.

[kecnry]

It exports as a region, yes. But I don't think we use the "region" terminology anywhere in the UI right now. What would help in the heading to make this more clear ("Subset (region)"? "Subset as region"?)

[bmorris3]

I used the UI to learn that it was a region by looking at the export formats, which are FITS, reg.

Maybe my confusion stems from the name "Export". If you're coming to this UI as a user without the glue background, you could interpret an "exported subset" to be:

• a cutout of the data within the selection in one or all viewers
• some metadata that specifies the pixel area marked with the red overlay in one or all viewers

So when I selected spectrum-viewer, expecting to "export" the collapsed spectrum from the spectrum-viewer, I was surprised to see reg as an available output format. (There are stacks of misconceptions here).

[cshanahan1]

I could add some text under the Subsets section heading?

[bmorris3]

I'm happy with:

• adding text under the Subsets header saying something like "Subsets will be exported as astropy regions" or similar
• rather than deselecting the viewer when a subset is selected, maybe let's auto-select the viewer where the subset is defined? That way, when you select this spatial subset, the flux-viewer gets selected and you know the exported file is a spatial subset.

[cshanahan1]

I'm happy with:

• adding text under the Subsets header saying something like "Subsets will be exported as astropy regions" or similar
• rather than deselecting the viewer when a subset is selected, maybe let's auto-select the viewer where the subset is defined? That way, when you select this spatial subset, the flux-viewer gets selected and you know the exported file is a spatial subset.

I feel like 2. would break the convention in the plugin that the (single) selected item is what is going to be exported, maybe?

[kecnry]

rather than deselecting the viewer when a subset is selected, maybe let's auto-select the viewer where the subset is defined? That way, when you select this spatial subset, the flux-viewer gets selected and you know the exported file is a spatial subset.
I feel like 2. would break the convention in the plugin that the (single) selected item is what is going to be exported, maybe?

Yes, this conflicts with the design. If a viewer is selected, you get the "screenshot" of that viewer. Eventually we also need batch export where you can select both a viewer (screenshot) AND a subset (region) resulting in a zip/tar file containing both.

Maybe adding short descriptions under each heading would be useful. For viewers: "export image of plot shown in viewer" and for subsets: "export subset definition as an astropy region", or similar.

[bmorris3]

I see what I was missing now, thanks. That sounds good! Maybe we could make the viewer selector say "screenshot from viewer"?

[kecnry]

Thanks! I think we can/should address some of @bmorris3's concerns further as we implement the rest of the plugin and get the final UI/UX review, but this works for me for now.

[bmorris3]

Awesome. 👌🏻