Model fitting residuals

[kecnry]

Description

This pull request implements the option to expose the residuals from a model fit as an additional entry in the data collection.

Open questions:

• should the residuals be automatically added to the viewer (if the switch to add the model is checked), should it have an independent switch, or should it not be added (current behavior)?

Screen.Recording.2022-11-22.at.2.09.27.PM.mov

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)?

[codecov]

Codecov Report

Base: 88.45% // Head: 88.48% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (18ec056) compared to base (5232ca0).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1864      +/-   ##
==========================================
+ Coverage   88.45%   88.48%   +0.02%     
==========================================
  Files          95       95              
  Lines       10533    10540       +7     
==========================================
+ Hits         9317     9326       +9     
+ Misses       1216     1214       -2     

Impacted Files	Coverage Δ
...igs/default/plugins/model_fitting/model_fitting.py	83.87% <100.00%> (+0.70%)	⬆️
jdaviz/core/template_mixin.py	92.54% <100.00%> (+0.01%)	⬆️

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 seems that during past refactors this kind of got left as an unnecessary artifact that only gets called once internally (and so never even entered the else statement in 797). This PR moves the remaining applicable logic to where it was called before.

[rosteen]

Works as expected in Specviz and Cubeviz, I'll give the code quick look-over in the morning before I approve.

[camipacifici]

Looks lovely! But I hit a traceback if I have two spectra loaded, I highlight a subset, and then I select it in the model fitting plugin.

---

ValueError Traceback (most recent call last)
File ~/opt/miniconda3/envs/pr1864/lib/python3.10/site-packages/ipywidgets/widgets/widget.py:756, in Widget._handle_msg(self, msg)
754 if 'buffer_paths' in data:
755 _put_buffers(state, data['buffer_paths'], msg['buffers'])
--> 756 self.set_state(state)
758 # Handle a state request.
759 elif method == 'request_state':

File ~/opt/miniconda3/envs/pr1864/lib/python3.10/site-packages/ipywidgets/widgets/widget.py:620, in Widget.set_state(self, sync_data)
615 self._send(msg, buffers=echo_buffers)
617 # The order of these context managers is important. Properties must
618 # be locked when the hold_trait_notification context manager is
619 # released and notifications are fired.
--> 620 with self._lock_property(**sync_data), self.hold_trait_notifications():
621 for name in sync_data:
622 if name in self.keys:

File ~/opt/miniconda3/envs/pr1864/lib/python3.10/contextlib.py:142, in _GeneratorContextManager.exit(self, typ, value, traceback)
140 if typ is None:
141 try:
--> 142 next(self.gen)
143 except StopIteration:
144 return False

File ~/opt/miniconda3/envs/pr1864/lib/python3.10/site-packages/traitlets/traitlets.py:1371, in HasTraits.hold_trait_notifications(self)
1369 for changes in cache.values():
1370 for change in changes:
-> 1371 self.notify_change(change)

File ~/opt/miniconda3/envs/pr1864/lib/python3.10/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/miniconda3/envs/pr1864/lib/python3.10/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/miniconda3/envs/pr1864/lib/python3.10/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 ~/opt/miniconda3/envs/pr1864/lib/python3.10/site-packages/jdaviz/core/template_mixin.py:874, in SubsetSelect._selected_changed(self, event)
872 def _selected_changed(self, event):
873 super()._selected_changed(event)
--> 874 self._update_has_subregions()

File ~/opt/miniconda3/envs/pr1864/lib/python3.10/site-packages/jdaviz/core/template_mixin.py:925, in SubsetSelect._update_has_subregions(self)
923 self.selected_has_subregions = False
924 else:
--> 925 self.selected_has_subregions = len(self.selected_obj.subregions) > 1

File ~/opt/miniconda3/envs/pr1864/lib/python3.10/functools.py:981, in cached_property.get(self, instance, owner)
979 val = cache.get(self.attrname, _NOT_FOUND)
980 if val is _NOT_FOUND:
--> 981 val = self.func(instance)
982 try:
983 cache[self.attrname] = val

File ~/opt/miniconda3/envs/pr1864/lib/python3.10/site-packages/jdaviz/core/template_mixin.py:936, in SubsetSelect.selected_obj(self)
932 # NOTE: we use reference names here instead of IDs since get_subsets_from_viewer requires
933 # that. For imviz, this will mean we won't be able to loop through each of the viewers,
934 # but the original viewer should have access to all the subsets.
935 for viewer_ref in self.viewer_refs:
--> 936 match = self.app.get_subsets_from_viewer(viewer_ref,
937 subset_type=subset_type).get(self.selected)
938 if match is not None:
939 return match

File ~/opt/miniconda3/envs/pr1864/lib/python3.10/site-packages/jdaviz/app.py:815, in Application.get_subsets_from_viewer(self, viewer_reference, data_label, subset_type)
812 else:
813 data_wcs = value.data.coords
--> 815 subregions_in_subset = _get_all_subregions(
816 np.where(value.to_mask() == True)[0], # noqa
817 data_wcs.spectral_axis)
819 regions[key] = subregions_in_subset
820 continue

File ~/opt/miniconda3/envs/pr1864/lib/python3.10/site-packages/jdaviz/app.py:742, in Application.get_subsets_from_viewer.._get_all_subregions(mask, spec_axis_data)
725 """
726 Return all subregions within a subset.
727
(...)
737 SpectralRegion object containing all subregions of the subset.
738 """
739 if len(mask) == 0:
740 # Mask should only be 0 if ApplyROI is used to incorrectly
741 # create subsets via the API
--> 742 raise ValueError("Mask has length 0, ApplyROI may have been used incorrectly")
744 current_edge = 0
745 combined_spec_region = None

ValueError: Mask has length 0, ApplyROI may have been used incorrectly

[rosteen]

Interesting - I didn't hit that error with multiple loaded spectra (the top two in the screenshot), but maybe I didn't replicate your workflow exactly.

[camipacifici]

Oh I see. Did some more tests and this is fine

this is also fine

but this is not fine

[camipacifici]

The solution for the Mosviz data dropdown backfired :( the residual is not appearing in the list

[kecnry]

@camipacifici - I wouldn't expect that bug was introduced here, but can you either provide more details to reproduce or test on main and confirm if it happens there as well or not? As for mosviz, that's a good catch, that'll definitely need to be fixed before merging this, so I'll mark this back as draft until I get a chance to fix assigning the mosviz row/plugin information.

[camipacifici]

@kecnry I confirm that the bug is in main too. I will create a ticket for it. Thanks!
#1868

[kecnry]

I think mosviz is now working correctly (although note that because of the small size of the viewers and the data menu now being attached in #1712, some resizing might be necessary in order to access the additional entries).

One slightly annoying behavior is that the residuals entry is not added to the viewer by default (as consistent with other viztools in this PR), but will be loaded by default on a row change. Should residuals be plotted by default for all viztools instead?

[camipacifici]

Looks great (disco mode included!)
I don't think I have an answer to your question. I will have to use it with a science case in mind and see what is annoying and what is nice to have. I am happy with it as is.

[rosteen]

LGTM, thanks.

[javerbukh]

Looks good! I did notice the mosviz behavior with row switching but if you're ok with that being follow-up work thats fine.

[kecnry]

I did notice the mosviz behavior with row switching but if you're ok with that being follow-up work thats fine.

I'd be happy to make the changes here if we knew the behavior we want... but I suspect we might need to use it a little while first to get a feel for the expected behavior (in which case deferring probably makes sense). I'll leave this open for a little while in case anyone has thoughts on that and will merge if I don't here anything by Monday will merge as-is and we can revise down the road if necessary.