Add Spatial Collapsed Spectrum selection to Line Analysis #1583

duytnguyendtn · 2022-08-18T20:04:12Z

Description

Previously, it was not possible to perform line analysis on the spectrum generated by Cubeviz' autocollapse for spatial subsets. This PR adds that feature.

BIG THANKS to @pllim for fixing the mysterious test failure!

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

kecnry · 2022-08-18T20:08:01Z

jdaviz/configs/specviz/plugins/line_analysis/line_analysis.py

+            self.spatial_subset = SubsetSelect(self,
+                                               'spatial_subset_items',
+                                               'collapsed_spectrum_selected',
+                                               default_text='Entire Cube Spectrum',
+                                               allowed_type='spatial')


I'd advocate for using the same traitlet names and default text as in model fitting so there is a consistent UX and API for the user between the two. If there are subtle differences that we want to capture in the UI, then the hint might be the easiest place to make that distinction. (Or we can always change model fitting to match if there is a good reason to make changes).

I'd still like to see this addressed/discussed before approving/merging!

I was going to address this in my "review phase" now that the CI is passing. I think I've mellowed on my original intent, so I'm happy to switch it back! Let me do that first and I'll rerequest review

I am not familiar with model fitting, so maybe @rosteen or @javerbukh would have opinions.

Effectively the distinction I tried to make is between selecting the spatial subset itself, or the spectrum generated from the spatial subset. Effectively, it looks identical on the front end, but it's slightly different here in the backend

@kecnry I've reverted those changes so they should match; let me know if it's sufficient, or if I missed something!

I do see your point, but just really want to strive for self-consistency across plugins, especially with attribute names and options since we're looking to expose these directly in #1401. This still differs from model fittings spatial dropdown (which is the same use-case: acting on the spectrum corresponding to the referenced spatial subset), in that model fitting has "Entire Cube" and this has "Entire Cube Spectrum". I definitely think they should be the same, and personally would vote for "Entire Cube" and add any clarification to the user in the dropdown hint and/or docs. But if others prefer being more verbose, then let's also make the change to model fitting.

Ahh, I misinterpreted your concern to be about the actual traitlets, not realizing you were also concerned about the labeling. I can understand your concern; I'll revert the labeling as well and we can defer this discussion to another day

Ok, I changed the default text/labeling as well. I reworked the hint to try and make it explicit. How does that look @kecnry ?

jdaviz/configs/specviz/plugins/line_analysis/tests/test_line_analysis.py

pllim · 2022-09-07T16:35:57Z

I rewrote your test and it passed on CI, so hopefully it will continue to pass and that wasn't a transient happy accident. 🤞

See duytnguyendtn#1

The original test case is perhaps too unrealistic to the point of triggering some edge case behavior where sometimes the calculation would give you a number and sometimes a NaN. I also found that even with the new test case, I need to reduce the width to avoid NaN.

TST: Use a slightly more realistic test case

pllim

Minor change log typo aside, it did work as advertised while I was investigating the CI failure, so approving. Thanks!

CHANGES.rst

Co-authored-by: P. L. Lim <[email protected]>

codecov · 2022-09-07T17:35:07Z

Codecov Report

Base: 86.35% // Head: 86.37% // Increases project coverage by +0.02% 🎉

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1583      +/-   ##
==========================================
+ Coverage   86.35%   86.37%   +0.02%     
==========================================
  Files          94       94              
  Lines        9410     9417       +7     
==========================================
+ Hits         8126     8134       +8     
+ Misses       1284     1283       -1

Impacted Files	Coverage Δ
...igs/specviz/plugins/line_analysis/line_analysis.py	`97.75% <100.00%> (+0.53%)`	⬆️

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.

…into linesubset

kecnry · 2022-09-08T13:34:43Z

jdaviz/configs/specviz/plugins/line_analysis/line_analysis.py

+        if self.config == 'cubeviz' and self.spatial_subset_selected != 'Entire Cube Spectrum':
+            # then we're acting on the auto-collapsed data in the spectrum-viewer
+            # of a spatial subset.  In the future, we may want to expose on-the-fly
+            # collapse options... but right now these will follow the settings of the
+            # spectrum-viewer itself
+            full_spectrum = self.app.get_data_from_viewer('spectrum-viewer',
+                                                          self.spatial_subset_selected)
+        else:
+            full_spectrum = self.dataset.selected_obj


this is currently duplicated logic from model fitting (I believe). That's probably fine for now, but if we start extending this pattern to more plugins, we should consider something like self.dataset.get_spatial_spectrum(self.spatial_subset_selected) to move the logic to a shared location.

kecnry

Seems to work well, thanks! (womp, womp, same CI failure that I thought was fixed by @pllim - as long as fixing that doesn't affect the implementation, I think this is ready to merge once CI passes).

duytnguyendtn · 2022-09-08T13:35:49Z

Oops something broke, one sec

duytnguyendtn · 2022-09-08T13:39:41Z

Ok that should fix it (at least it's passing locally). Let's merge once these tests pass

duytnguyendtn · 2022-09-08T14:52:29Z

Thanks so much for the reviews @kecnry @pllim !!

github-actions bot added the specviz label Aug 18, 2022

duytnguyendtn added this to the 2.9 milestone Aug 18, 2022

kecnry reviewed Aug 18, 2022

View reviewed changes

duytnguyendtn force-pushed the linesubset branch 3 times, most recently from e9e3328 to 6566513 Compare August 19, 2022 21:58

duytnguyendtn commented Aug 19, 2022

View reviewed changes

jdaviz/configs/specviz/plugins/line_analysis/tests/test_line_analysis.py Show resolved Hide resolved

rosteen modified the milestones: 2.9, 2.10 Aug 24, 2022

pllim modified the milestones: 2.10, 2.11 Aug 26, 2022

duytnguyendtn added 13 commits September 2, 2022 16:13

Add Collapsed Spectrum option to Line Analysis PR

1715c91

Fix Typo

3683d01

Changelog

27f6916

Add Spatial Subset test

8867d09

Don't overwrite same subset for tests

33aab9a

Update spatial subset test values to be within dataset bounds

0a05a74

Add larger cube to Line Analysis tests

5ca0c5d

Remove unneeded test cube fixture

9f290eb

Add missing imports

21ae02e

Filter WCS warning

f3b6703

typo

1f59f18

Missing float conversion

98dc53d

Swap unit nan check for dimensionless check

0ecf6b0

duytnguyendtn force-pushed the linesubset branch from 6566513 to 0ecf6b0 Compare September 2, 2022 20:14

pllim reviewed Sep 7, 2022

View reviewed changes

jdaviz/configs/specviz/plugins/line_analysis/tests/test_line_analysis.py Outdated Show resolved Hide resolved

pllim mentioned this pull request Sep 7, 2022

EXP: Debug PR 1583 #1627

Closed

9 tasks

TST: Use a slightly more realistic test case.

2dc9faa

Merge pull request #1 from pllim/pr1583-debug

1af7936

TST: Use a slightly more realistic test case

duytnguyendtn marked this pull request as ready for review September 7, 2022 17:21

duytnguyendtn requested review from rosteen, javerbukh and ojustino as code owners September 7, 2022 17:21

pllim approved these changes Sep 7, 2022

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

pllim added cubeviz Ready for final review labels Sep 7, 2022

Changelog Typo

602b486

Co-authored-by: P. L. Lim <[email protected]>

duytnguyendtn added 3 commits September 7, 2022 15:38

Revert Spatial Subset namechange

e1075c1

Merge branch 'linesubset' of https://github.com/duytnguyendtn/jdaviz …

5211da7

…into linesubset

Revert labels

b52ba14

kecnry reviewed Sep 8, 2022

View reviewed changes

kecnry approved these changes Sep 8, 2022

View reviewed changes

Rename test traitlet

8bb5178

duytnguyendtn merged commit e9df543 into spacetelescope:main Sep 8, 2022

duytnguyendtn deleted the linesubset branch September 8, 2022 14:52

This was referenced Sep 8, 2022

Fix Typo in Line Analysis #1631

Merged

Catch known test warning #1632

Merged

user API for compass plugin #1643

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Spatial Collapsed Spectrum selection to Line Analysis #1583

Add Spatial Collapsed Spectrum selection to Line Analysis #1583

duytnguyendtn commented Aug 18, 2022 •

edited by pllim

Loading

kecnry Aug 18, 2022

kecnry Sep 7, 2022

duytnguyendtn Sep 7, 2022

pllim Sep 7, 2022

duytnguyendtn Sep 7, 2022

duytnguyendtn Sep 7, 2022

kecnry Sep 8, 2022

duytnguyendtn Sep 8, 2022

duytnguyendtn Sep 8, 2022

pllim commented Sep 7, 2022

pllim left a comment

codecov bot commented Sep 7, 2022 •

edited

Loading

kecnry Sep 8, 2022

kecnry left a comment •

edited

Loading

duytnguyendtn commented Sep 8, 2022

duytnguyendtn commented Sep 8, 2022

duytnguyendtn commented Sep 8, 2022 •

edited

Loading

Add Spatial Collapsed Spectrum selection to Line Analysis #1583

Add Spatial Collapsed Spectrum selection to Line Analysis #1583

Conversation

duytnguyendtn commented Aug 18, 2022 • edited by pllim Loading

Description

Checklist for package maintainer(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Sep 7, 2022

pllim left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 7, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

kecnry left a comment • edited Loading

Choose a reason for hiding this comment

duytnguyendtn commented Sep 8, 2022

duytnguyendtn commented Sep 8, 2022

duytnguyendtn commented Sep 8, 2022 • edited Loading

duytnguyendtn commented Aug 18, 2022 •

edited by pllim

Loading

codecov bot commented Sep 7, 2022 •

edited

Loading

kecnry left a comment •

edited

Loading

duytnguyendtn commented Sep 8, 2022 •

edited

Loading