Add composite subset fixes #2291
Conversation
javerbukh
requested review from
duytnguyendtn,
rosteen,
pllim,
kecnry,
bmorris3,
haticekaratay and
cshanahan1
as code owners
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2291 +/- ##
==========================================
+ Coverage 90.91% 90.94% +0.03%
==========================================
Files 152 152
Lines 17028 17140 +112
==========================================
+ Hits 15481 15588 +107
- Misses 1547 1552 +5
☔ View full report in Codecov by Sentry. |
| @@ -377,7 +378,7 @@ def add_data(self, data, color=None, alpha=None, **layer_state): | |||
| # that new data entries (from model fitting or gaussian smooth, etc) will only be spectra | |||
| # and all subsets affected will be spectral | |||
| for layer in self.state.layers: | |||
| if "Subset" in layer.layer.label and layer.layer.data.label == data.label: | |||
| if get_subset_type(layer.layer) == 'spectral' and layer.layer.data.label == data.label: | |||
There was a problem hiding this comment.
I think get_subset_type assumes the input is a subset... does the fact that this works depend on the assumption that spectral is defined first in that function (do we need some way to still detect if the layer is a subset at all vs actual data)?
There was a problem hiding this comment.
get_subset_type has a check to start if the layer.layer object has attribute "subset_state" and if not it returns None. I added a check to this if statement though to make it more clear that this should only accept layer's associated with subsets.
| assert len([m for m in spectrum_viewer.figure.marks if isinstance(m, ShadowSpatialSpectral)]) == 1 # noqa | ||
| assert len([m for m in spectrum_viewer.figure.marks if isinstance(m, ShadowSpatialSpectral)]) == 2 # noqa |
There was a problem hiding this comment.
this (and the next set of changes to line 229) is only temporary until #2292 is addressed, right?
There was a problem hiding this comment.
Assuming Gaussian Smooth stays a cube, this is the correct number for the amount of ShadowSpatialSpectral marks. A spatial subset is applied to the flux cube AND the smoothed cube, which creates two collapsed spectra in the spectrum viewer. Each of those spectra then has a spectral subset applied to them, creating 2 shadow marks. If you add another spectral subset, that number goes up to 4. If you delete the smooth cube, that number goes back down to 2 since you still have the flux cube's spatial subset intersecting with the two spectral subsets.
Co-authored-by: Brett M. Morris <morrisbrettm@gmail.com>
|
Thank you for the reviews! I'll merge once CI tests pass. |
Description
This pull request continues the work that @kecnry started in #2207 and I continued in #2266. This fixes the first bug found in #2182 (comment) . It also applies the fix in coords info so that the collapsed composite spatial subset can be hovered over.
Fixes #
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-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.
triviallabel.