Cubeviz spectral regions fixes #1046

rosteen · 2022-01-20T18:26:55Z

Description

This allows Cubeviz to successfully return spectral regions with multiple subregions via app.get_subsets_from_viewer. It also makes the returned spectral regions from Cubeviz SpectralRegions, to match the output when the method is used in Specviz. Some changes were also required to the moment maps plugin to work with this change - the rest of the plugins seemed ok, but I'm leaving this as draft for now because I think this may be a good time to make one or two other subset-related tweaks to the moment maps UI (and potentially other plugins).

Fixes #959

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? Milestone is only currently required for PRs related to Imviz MVP.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

rosteen · 2022-01-21T16:02:06Z

This is ready for review, but will require astropy/specutils#911 for the tests to pass. I'm hoping to do a specutils release early next week so we can merge this before the end of the week.

pllim · 2022-01-21T16:08:04Z

Should I withhold review until specutils is released then?

rosteen · 2022-01-21T16:08:58Z

I was thinking that it's reviewable now, we can just wait to hit the merge button until tests pass.

pllim · 2022-01-21T16:11:51Z

But we have to install astropy/specutils#911 to review this, right?

rosteen · 2022-01-21T16:16:16Z

No, unless you have an overriding urge to use the Unit Conversion plugin to change units to frequency space before testing it.

pllim · 2022-01-21T16:19:37Z

Oh, you mean this? 😸

BUG: Unit Conversion to Hz breaks Line Lists plugin #1034

kecnry

Seems to work well and as expected (pending tests passing) and is definitely a huge improvement over returning a messy nested object.

Note that the scope here only applies to spectral regions - image regions work when adding multiple subregions but still break when using "remove" logic, for instance.

kecnry · 2022-01-21T21:39:36Z

jdaviz/app.py

-                    continue
-                elif subset_type == "spatial" and this_type == RangeSubsetState:
-                    continue
+                if subset_type == "spectral" or viewer_reference == "spectrum-viewer":


is hardcoding the viewer-reference string ever going to cause issues (also in line 618)? (See #1038, for example). As far as I can tell, we always currently use "spectrum-viewer" for spectrum viewers. We could check if viewer is a SpecvizProfileView instance, but I'm not sure if that's better or not.

As long as @havok2063 does not overwrite this on MAST side, we should be 99% safe...

We currently don't override anything within the viewer-area section of the configurations. I don't think we have any plans to but one never knows. Items in settings, toolbar, and tray we do reconfigure.

Checking for SpecvizProfileView seems like it's probably the more stable solution for now.

Actually, better to look for BqplotProfileView. I'll make that change shortly.

pllim

Is Cubeviz moment map plugin the only thing affected by this? Seems like a very significant change. No other plugins currently access this API and expecting the old object class?

jdaviz/configs/cubeviz/plugins/moment_maps/moment_maps.py

jdaviz/tests/test_subsets.py

pllim · 2022-01-24T19:29:33Z

CHANGES.rst

@@ -30,6 +30,9 @@ Bug Fixes
 Cubeviz
 ^^^^^^^

+- Subsets from the spectrum viewer are now returned as SpectralRegion objects, and


The first entry is technically API change, not just a bug fix. I propose we split this entry into different "API change" and "Bug fix" entries.

jdaviz/tests/test_subsets.py

pllim · 2022-01-24T19:33:07Z

jdaviz/app.py

+                temp_data = self.get_data_from_viewer(viewer_reference, value.label)
+                if isinstance(temp_data, Spectrum1D):
+                    # Note that we look for mask == False here, rather than True above,
+                    # because specutils masks are the reverse of Glue (of course)


FWIW, specutils at least follows Numpy masked array convention (where mask==True means the pixel is masked). 🤷

jdaviz/app.py

pllim · 2022-01-24T19:39:04Z

p.s. You said this fixes "multiple subregions" use case. Maybe add a test for that? 😅

Thanks!

rosteen · 2022-01-27T21:37:11Z

@pllim I merged your suggestions and added a test for a two-region Subset, as well as updating the specutils pin to the new release. I think this is good for final review.

pllim · 2022-01-28T14:08:16Z

There is a conflict in the change log, so this needs rebasing. Thanks!

rosteen · 2022-01-31T14:27:32Z

Rebased!

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

Fix codestyle

codecov · 2022-01-31T15:20:55Z

Codecov Report

Attention: Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 74.22%. Comparing base (bd81723) to head (0cd349d).
Report is 2727 commits behind head on main.

Files	Patch %	Lines
...configs/cubeviz/plugins/moment_maps/moment_maps.py	50.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1046      +/-   ##
==========================================
+ Coverage   74.03%   74.22%   +0.18%     
==========================================
  Files          75       75              
  Lines        5639     5637       -2     
==========================================
+ Hits         4175     4184       +9     
+ Misses       1464     1453      -11

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

pllim

Works as advertised. Thanks!

github-actions bot added cubeviz testing labels Jan 20, 2022

rosteen changed the title ~~Cubeviz gimme regions~~ Cubeviz spectral regions fixes Jan 21, 2022

rosteen marked this pull request as ready for review January 21, 2022 16:01

rosteen requested review from duytnguyendtn, ibusko, javerbukh, kecnry, ojustino and pllim as code owners January 21, 2022 16:01

pllim added this to the 2.3 milestone Jan 21, 2022

pllim added the Upstream fix required label Jan 21, 2022

kecnry reviewed Jan 21, 2022

View reviewed changes

pllim reviewed Jan 24, 2022

View reviewed changes

rosteen force-pushed the cubeviz-gimme-regions branch from 25c0e73 to 3211fbd Compare January 27, 2022 21:17

rosteen added Ready for final review and removed Upstream fix required labels Jan 27, 2022

rosteen force-pushed the cubeviz-gimme-regions branch from 3211fbd to e445fbd Compare January 31, 2022 14:16

rosteen added 2 commits January 31, 2022 10:15

Fix bug with retrieving spectral regions in Cubeviz

066dcfb

Update moment maps subset selection to work with returned SpectralRegion

0ead325

rosteen and others added 8 commits January 31, 2022 10:15

Remove unused imports

7527f06

Add changelog entry

bc5c200

Fix inverted regions

260c014

Check for viewer type instead of ref string

435fb77

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

Update specutils version requirement

f5c7e04

Apply suggestions from code review

48f2f96

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

Move part of changelog entry to API changes

e694a9e

Add multi-subregion subset test

0cd349d

Fix codestyle

rosteen force-pushed the cubeviz-gimme-regions branch from e445fbd to 0cd349d Compare January 31, 2022 15:15

kecnry approved these changes Jan 31, 2022

View reviewed changes

pllim approved these changes Jan 31, 2022

View reviewed changes

pllim merged commit e30907a into spacetelescope:main Jan 31, 2022

pllim mentioned this pull request Feb 1, 2022

Remove dependency on spectral-cube #1006

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cubeviz spectral regions fixes #1046

Cubeviz spectral regions fixes #1046

rosteen commented Jan 20, 2022 •

edited by pllim

Loading

rosteen commented Jan 21, 2022

pllim commented Jan 21, 2022

rosteen commented Jan 21, 2022

pllim commented Jan 21, 2022

rosteen commented Jan 21, 2022

pllim commented Jan 21, 2022

kecnry left a comment

kecnry Jan 21, 2022

pllim Jan 21, 2022

havok2063 Jan 24, 2022

rosteen Jan 24, 2022

rosteen Jan 24, 2022

pllim left a comment •

edited

Loading

pllim Jan 24, 2022

pllim Jan 24, 2022

pllim commented Jan 24, 2022

rosteen commented Jan 27, 2022

pllim commented Jan 28, 2022

rosteen commented Jan 31, 2022

codecov bot commented Jan 31, 2022 •

edited

Loading

pllim left a comment

Cubeviz spectral regions fixes #1046

Cubeviz spectral regions fixes #1046

Conversation

rosteen commented Jan 20, 2022 • edited by pllim Loading

Description

Checklist for package maintainer(s)

rosteen commented Jan 21, 2022

pllim commented Jan 21, 2022

rosteen commented Jan 21, 2022

pllim commented Jan 21, 2022

rosteen commented Jan 21, 2022

pllim commented Jan 21, 2022

kecnry left a comment

Choose a reason for hiding this comment

kecnry Jan 21, 2022

Choose a reason for hiding this comment

pllim Jan 21, 2022

Choose a reason for hiding this comment

havok2063 Jan 24, 2022

Choose a reason for hiding this comment

rosteen Jan 24, 2022

Choose a reason for hiding this comment

rosteen Jan 24, 2022

Choose a reason for hiding this comment

pllim left a comment • edited Loading

Choose a reason for hiding this comment

pllim Jan 24, 2022

Choose a reason for hiding this comment

pllim Jan 24, 2022

Choose a reason for hiding this comment

pllim commented Jan 24, 2022

rosteen commented Jan 27, 2022

pllim commented Jan 28, 2022

rosteen commented Jan 31, 2022

codecov bot commented Jan 31, 2022 • edited Loading

Codecov Report

pllim left a comment

Choose a reason for hiding this comment

rosteen commented Jan 20, 2022 •

edited by pllim

Loading

pllim left a comment •

edited

Loading

codecov bot commented Jan 31, 2022 •

edited

Loading