Specviz2d to parse NIRSpec level 2 s2d

[pllim]

Description

This pull request is an attempt to parse Level 2 NIRSpec s2d files.

TODO

• I cannot find any valid WCS in the given file to work with. What to do now? Just don't pass in the WCS at all.
• Works around Cubeviz doesn't load Flux units #1625
• Spectrum 1D collapse fails. How to fix this?
• Use spectral extraction defaults: fix case where background exactly on edge #1633 and remove fallback.
• Why is info bar cut off a little?
• Wait for a new glue-astronomy release and update minversion pin in setup.cfg.

Blocked by

Please install dev v0.5.1 or later of glue-astronomy to get these fixes from upstream:

• Return empty string instead of crashing when UCD_TO_SPECTRAL_NAME lookup failed glue-viz/glue-astronomy#77
• BUG: Parse units for Spectrum1D for all occasions glue-viz/glue-astronomy#78

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 change log needed? If yes, is it added to CHANGES.rst?
• Is a milestone set?
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

[codecov]

Codecov Report

Base: 87.15% // Head: 87.11% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (8c16675) compared to base (1aeb356).
Patch coverage: 54.90% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1608      +/-   ##
==========================================
- Coverage   87.15%   87.11%   -0.04%     
==========================================
  Files          95       95              
  Lines        9854     9890      +36     
==========================================
+ Hits         8588     8616      +28     
- Misses       1266     1274       +8     

Impacted Files	Coverage Δ
...z/configs/default/plugins/data_tools/data_tools.py	60.60% <0.00%> (+5.34%)	⬆️
jdaviz/configs/mosviz/plugins/parsers.py	87.96% <0.00%> (-1.58%)	⬇️
jdaviz/configs/specviz2d/helper.py	84.61% <0.00%> (+5.66%)	⬆️
jdaviz/configs/mosviz/plugins/viewers.py	79.02% <71.79%> (-1.82%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kecnry]

It looks like the spectral-extraction is just failing because the background region is out-of-bounds. If you change the background manually, does the spectral extraction succeed?

The error statement itself should be fixed by astropy/specreduce#127, but we should probably also improve the automatic logic to result in a one-sided default background in cases like this where the extraction region is near the top or bottom. I reported this as #1620.

[pllim]

I added this for my own sanity here though it is technically out of scope as per original request.

[kecnry]

Is there anything modified from the two other instances of this (imviz and cubeviz)? I really think we should refactor and move the generic logic into either the plugin (preferred, imo) or a shared viewer class.

[pllim]

Yes, for this one, I don't think we can ever render sky coordinates. So if you want it to look pretty, it would be actually a new plugin without the sky coordinates field in vue template, or something.

Imviz has to worry about multiple 2D images. Cubeviz has to worry about 3D cube with 2D overlays. And Specviz2d has to worry about 2D spectra. So, everyone is a little different.

[kecnry]

Why is info bar cut off a little?

I am guessing the coordinates display is assuming the taller app-toolbar height from imviz and cubeviz, but I don't think specviz2d warrants the increased height unless its going to use all the space. I'd vote for this to be moved to a separate PR then so we can try to generalize it and have the displayed info be responsive to the type of underlying data. But we can also have refactoring that as a follow-up effort if you'd prefer and can find an easy way to at least generalize the height.

[pllim]

If there is strong opposition to do this now, I can take it back out but not till after we know we fixed the parsing for sure because having this is nice for sanity check.

[pllim]

Cami wants this to be kept, so I opened #1645 as a follow-up issue w.r.t. refactoring. Thanks!

[pllim]

FWIW, this is the actual fix for the original issue (plus upstream fixes).

[pllim]

@kecnry , this PR is basically done. Just waiting for upstream fix. Does the diff look okay to you?

[kecnry]

Just waiting for upstream fix. Does the diff look okay to you?

Nothing sticks out to me. Happy to do a full review once the upstream fix is in!

[pllim]

This should be ready for review.

Coverage decreased because we don't really have good test cases for Specviz2D and some things like snackbar cannot really be tested. Adding better tests is out of scope here.

[kecnry]

Works well on the sample file I tried. I agree that the mouseover and snackbar coverage might not be necessary (and is difficult), but it would probably be nice to get the parser lines covered with an s2d file as a test case?

[kecnry]

this might have implications for the MAST embed.

[pllim]

@havok2063 , is this setting change a problem for you?

[javerbukh]

I was able to load NIRSpec level 2 s2d data from MAST, so I am approving. However, I was able to trigger a traceback when trying to load that s2d data from our Import Data button, so I will open a new issue for that (traceback at the end of this comment). Also, when trying to use the Spectral Extraction PR, I was not able to see a trace at the end of the plugin because of the following error: ERROR from specreduce: MaskError('Cannot convert masked element to a Python int.'). Am I correct in assuming that is a Specreduce bug @kecnry ?

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
File ~/opt/anaconda3/envs/test_pl_pr/lib/python3.9/site-packages/ipyvue/VueTemplateWidget.py:60, in Events._handle_event(self, _, content, buffers)
     58     getattr(self, "vue_" + event)(data, buffers)
     59 else:
---> 60     getattr(self, "vue_" + event)(data)

File ~/Documents/jdaviz_dev/jdaviz/jdaviz/app.py:1258, in Application.vue_data_item_selected(self, event)
   1255 else:
   1256     selected_items = {k: v for k, v in viewer_item['selected_data_items'].items() if k != item_id}  # noqa
-> 1258 self._update_selected_data_items(viewer_id, selected_items)

File ~/Documents/jdaviz_dev/jdaviz/jdaviz/app.py:1328, in Application._update_selected_data_items(self, viewer_id, selected_items)
   1323 viewer.add_data(data, percentile=95)
   1325 add_data_message = AddDataMessage(data, viewer,
   1326                                   viewer_id=viewer_id,
   1327                                   sender=self)
-> 1328 self.hub.broadcast(add_data_message)
   1330 for layer in viewer.layers:
   1331     if layer.layer.data.label == label:

File ~/opt/anaconda3/envs/test_pl_pr/lib/python3.9/site-packages/glue/core/hub.py:215, in Hub.broadcast(self, message)
    213 logging.getLogger(__name__).info("Broadcasting %s", message)
    214 for subscriber, handler in self._find_handlers(message):
--> 215     handler(message)

File ~/Documents/jdaviz_dev/jdaviz/jdaviz/core/template_mixin.py:1370, in DatasetSelect._on_data_changed(self, msg)
   1367 manual_items = [{'label': label} for label in self.manual_options]
   1368 self.items = manual_items + [_dc_to_dict(data) for data in self.app.data_collection
   1369                              if self._is_valid_item(data)]
-> 1370 self._apply_default_selection()
   1371 # future improvement: only clear cache if the selected data entry was changed?
   1372 self._clear_cache(*self._cached_properties)

File ~/Documents/jdaviz_dev/jdaviz/jdaviz/core/template_mixin.py:484, in SelectPluginComponent._apply_default_selection(self)
    481     return
    483 if self.default_mode == 'first':
--> 484     self.selected = self.labels[0] if len(self.labels) else ''
    485 elif self.default_mode == 'default_text':
    486     self.selected = self._default_text if self._default_text else ''

File ~/Documents/jdaviz_dev/jdaviz/jdaviz/core/template_mixin.py:239, in BasePluginComponent.__setattr__(self, attr, value, force_super)
    236 if attr[0] == '_' or force_super or attr not in self._plugin_traitlets.keys():
    237     return super().__setattr__(attr, value)
--> 239 return setattr(self._plugin, self._plugin_traitlets.get(attr), value)

File ~/opt/anaconda3/envs/test_pl_pr/lib/python3.9/site-packages/traitlets/traitlets.py:715, in TraitType.__set__(self, obj, value)
    713     raise TraitError('The "%s" trait is read-only.' % self.name)
    714 else:
--> 715     self.set(obj, value)

File ~/opt/anaconda3/envs/test_pl_pr/lib/python3.9/site-packages/traitlets/traitlets.py:704, in TraitType.set(self, obj, value)
    700     silent = False
    701 if silent is not True:
    702     # we explicitly compare silent to True just in case the equality
    703     # comparison above returns something other than True/False
--> 704     obj._notify_trait(self.name, old_value, new_value)

File ~/opt/anaconda3/envs/test_pl_pr/lib/python3.9/site-packages/traitlets/traitlets.py:1374, in HasTraits._notify_trait(self, name, old_value, new_value)
   1373 def _notify_trait(self, name, old_value, new_value):
-> 1374     self.notify_change(
   1375         Bunch(
   1376             name=name,
   1377             old=old_value,
   1378             new=new_value,
   1379             owner=self,
   1380             type="change",
   1381         )
   1382     )

File ~/opt/anaconda3/envs/test_pl_pr/lib/python3.9/site-packages/ipywidgets/widgets/widget.py:686, in Widget.notify_change(self, change)
    683     if name in self.keys and self._should_send_property(name, getattr(self, name)):
    684         # Send new state to front-end
    685         self.send_state(key=name)
--> 686 super(Widget, self).notify_change(change)

File ~/opt/anaconda3/envs/test_pl_pr/lib/python3.9/site-packages/traitlets/traitlets.py:1386, in HasTraits.notify_change(self, change)
   1384 def notify_change(self, change):
   1385     """Notify observers of a change event"""
-> 1386     return self._notify_observers(change)

File ~/opt/anaconda3/envs/test_pl_pr/lib/python3.9/site-packages/traitlets/traitlets.py:1431, in HasTraits._notify_observers(self, event)
   1428 elif isinstance(c, EventHandler) and c.name is not None:
   1429     c = getattr(self, c.name)
-> 1431 c(event)

File ~/Documents/jdaviz_dev/jdaviz/jdaviz/configs/specviz2d/plugins/spectral_extraction/spectral_extraction.py:172, in SpectralExtraction._trace_dataset_selected(self, msg)
    168 if not hasattr(self, 'trace_dataset'):
    169     # happens when first initializing plugin outside of tray
    170     return
--> 172 width = self.trace_dataset.selected_obj.shape[0]
    173 # estimate the pixel number by taking the median of the brightest pixel index in each column
    174 brightest_pixel = int(np.median(np.argmax(self.trace_dataset.selected_obj.flux, axis=0)))

File ~/opt/anaconda3/envs/test_pl_pr/lib/python3.9/functools.py:993, in cached_property.__get__(self, instance, owner)
    991 val = cache.get(self.attrname, _NOT_FOUND)
    992 if val is _NOT_FOUND:
--> 993     val = self.func(instance)
    994     try:
    995         cache[self.attrname] = val

File ~/Documents/jdaviz_dev/jdaviz/jdaviz/core/template_mixin.py:1305, in DatasetSelect.selected_obj(self)
   1301 if viewer_ref is None:
   1302     # image viewers might not have a reference, but get_data_from_viewer
   1303     # does not take id
   1304     continue
-> 1305 match = self.app.get_data_from_viewer(viewer_ref, data_label=self.selected)
   1306 if match is not None:
   1307     if hasattr(match, 'get_object'):
   1308         # cube viewers return the data collection instance from get_data_from_viewer

File ~/Documents/jdaviz_dev/jdaviz/jdaviz/app.py:652, in Application.get_data_from_viewer(self, viewer_reference, data_label, cls, include_subsets)
    650     layer_data = layer_data.get_object()
    651 elif cls == Spectrum1D:
--> 652     layer_data = layer_data.get_object(cls=cls,
    653                                        statistic=statistic)
    654 else:
    655     layer_data = layer_data.get_object(cls=cls)

File ~/opt/anaconda3/envs/test_pl_pr/lib/python3.9/site-packages/glue/core/data.py:297, in BaseData.get_object(self, cls, **kwargs)
    292         raise ValueError('Specify the object class to use with cls= - supported '
    293                          'classes are:' + format_choices(data_translator.supported_classes))
    295 handler, _ = data_translator.get_handler_for(cls)
--> 297 return handler.to_object(self, **kwargs)

File ~/opt/anaconda3/envs/test_pl_pr/lib/python3.9/site-packages/glue_astronomy/translators/spectrum1d.py:216, in Specutils1DHandler.to_object(self, data_or_subset, attribute, statistic)
    213     kwargs = {'spectral_axis': data.coords.spectral_axis}
    215 else:
--> 216     raise TypeError('data.coords should be an instance of WCS '
    217                     'or SpectralCoordinates')
    219 # Copy over metadata
    220 kwargs['meta'] = data.meta.copy()

TypeError: data.coords should be an instance of WCS or SpectralCoordinates

[kecnry]

@javerbukh as long as that happened when the window went out of the bounds of the image, then that is a known bug and either fixed in specreduce main or in an open PR. If it was triggered in a different scenario, please file and issue/ticket in specreduce. Either way, I'd agree that it is out of scope here.

[pllim]

@javerbukh , I think I fixed the Load Data button. Please re-review. Thanks!

(Also you caught me never using that button.)

[javerbukh]

Re-reviewed and it works as expected, thanks!