Remove dependency on spectral-cube #1006

pllim · 2021-12-14T23:38:07Z

Description

This pull request is to remove dependency on spectral-cube but to do that we also need to refactor the Collapse plugin.

Fixes #922

Blocked by:

Decision on Collapse refactoring, e.g., do we even need to collapse to get spatial-spectral data?

spectral_slab does not handle frequency limits astropy/specutils#909

New specutils release beyond v1.5.0 for Spectrum1D.collapse().
Fix CI

Blocks:

Cubeviz parser: Refactor parser, add ndarray support, metadata viewer #1040

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

jdaviz/configs/default/plugins/collapse/collapse.py

pllim · 2021-12-14T23:48:26Z

jdaviz/configs/default/plugins/collapse/collapse.py

+
+        # FIXME: Does not work.
+        # collapsed_spec = spec.collapse(self.selected_func.lower(), axis=self.selected_axis)
+        # data = Data(coords=collapsed_spec.wcs)


I am not sure how to use specutils.Spectrum1D.collapse to get spatial-spectral object correctly here. Particularly, the axis order might not be the same as spectral-cube; not sure, as the warnings about flipping axis from specutils is very confusing.

But more importantly, do we really need so many options for axis if people only ever wants to collapse to get spatial-spatial?

In offline discussion we decided that giving "spectral" and "spatial" collapse options rather than numerical axes is the way to go here. None of us could think of a use case for collapsing only one spatial axis. FYI the specutils collapse method should take those two strings as valid axis inputs, so it is hopefully easy to implement.

As discussed in 2021-12-16 tag-up, "spectral" collapse already happens automatically on load, so having it also in the plugin would be redundant.

I forgot what's the conclusion here. Do we still support "spectral" collapse or just get rid of it?

I would say leave it in 🤷

I cannot make sense of the previous implementation using spectral-cube, so I removed "axis" option altogether. #yolo

rosteen · 2021-12-20T14:39:45Z

@pllim I'm hoping that astropy/specutils#911 resolves the spectral_slab blocker for this.

codecov · 2022-02-01T22:22:51Z

Codecov Report

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

Project coverage is 75.42%. Comparing base (45dc2e1) to head (dcbe93d).
Report is 2660 commits behind head on main.

Files	Patch %	Lines
...daviz/configs/default/plugins/collapse/collapse.py	88.23%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
+ Coverage   75.32%   75.42%   +0.09%     
==========================================
  Files          80       80              
  Lines        6059     6042      -17     
==========================================
- Hits         4564     4557       -7     
+ Misses       1495     1485      -10

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

pllim · 2022-02-01T22:23:44Z

docs/known_bugs.rst

@@ -111,11 +111,6 @@ try a different OS/browser combo.
 Specviz
 -------

-Collapse Plugin spectral bounds do not match selected region


This entry is misleading because Specviz does not load Collapse and there is already a similar entry above for Cubeviz.

pllim · 2022-02-01T22:24:18Z

jdaviz/configs/default/plugins/collapse/collapse.py

@@ -16,28 +14,15 @@
 __all__ = ['Collapse']


-spaxel = u.def_unit('spaxel', 1 * u.Unit(""))


This is not used. Not sure why it is here. It is also defined in at least two other places in this package but removing those is out of scope here.

pllim · 2022-02-01T22:25:20Z

jdaviz/configs/default/plugins/collapse/collapse.py

@@ -108,14 +88,14 @@ def vue_list_subsets(self, event):
        temp_dict = {}
        # Attempt to filter out spatial subsets
        for key, region in temp_subsets.items():
-            if type(region) == RectanglePixelRegion:
+            if type(region) == SpectralRegion:


I think this was overlooked in #1046 ?

pllim · 2022-02-01T22:28:13Z

jdaviz/configs/default/plugins/collapse/collapse.py


        snackbar_message = SnackbarMessage(
            f"Data set '{self._selected_data.label}' collapsed successfully.",
            color="success",
            sender=self)
        self.hub.broadcast(snackbar_message)

-        if self.selected_axis == 0 and self.selected_viewer != 'None':
+        # Spatial-spatial image only.
+        if self.selected_viewer != 'None':
            # replace the contents in the selected viewer with the results from this plugin
            self.app.add_data_to_viewer(self.viewer_to_id.get(self.selected_viewer),


@kecnry , I noticed something weird where while the collapsed image shows in the selected viewer, I am unable to use Layers menu to change the display. I have to deselect it from Data and re-select for Layers to work again. Not sure if this is a known issue or not.

Also, the spectral Subset somehow colors the whole collapsed image in the tint of the Subset color. I can disable it by clicking the eye icon on the Layers dropdown. I think this is a separate known issue already.

I know both of these are known issues. The second I think is expected behavior (so long as the current slice is within the subset), but I agree that we may want to reconsider that default once the slice indicator in #1013 is merged as there will already be visual indication of the slice being in a subset.

p.s. While it is expected behavior, I wonder if it is the wrong behavior. I cannot see the image properly with the tint all over it, but yes, it is a separate issue for another time.

pllim · 2022-02-01T22:29:02Z

jdaviz/configs/default/plugins/collapse/tests/test_collapse.py

    coll.add_replace_results = False
    coll.vue_collapse()

    assert len(dc) == 2
    assert dc[1].label == 'Collapsed 1 test'
-    assert len(dc.external_links) == 3
+    assert len(dc.external_links) == 2


The results changed here but I don't know what it means. Plugin seems to work though. 🤷

pllim · 2022-02-01T22:29:25Z

jdaviz/tests/test_subsets.py

@@ -70,8 +70,6 @@ def test_region_from_subset_profile(jdaviz_app, spectral_cube_wcs):
    subsets = jdaviz_app.get_subsets_from_viewer('spectrum-viewer', subset_type='spectral')
    reg = subsets.get('Subset 1')

-    print(reg)


Leftover debugging from #1046 .

javerbukh · 2022-02-08T13:26:55Z

While testing this, I noticed that collapsed data is in the wrong orientation. Is this a working as expected or a bug?

jdaviz/configs/default/plugins/collapse/collapse.vue

kecnry · 2022-02-09T16:19:00Z

While testing this, I noticed that collapsed data is in the wrong orientation.

This rotation does not seem to occur in main (when choosing axis=0).

pllim · 2022-02-09T21:18:16Z

jdaviz/configs/default/plugins/collapse/collapse.py

+        if data_label not in self.data_collection.labels:
+            return
+        self._selected_data = self.data_collection[self.data_collection.labels.index(data_label)]
+        self._selected_cube = self._selected_data.get_object(cls=Spectrum1D, statistic=None)


Note: I feel like there is no point to extract the cube multiple times across the plugin. Here, I change it to extracting it only once when data is selected.

pllim · 2022-02-09T21:20:00Z

jdaviz/configs/default/plugins/collapse/collapse.py

+            warnings.filterwarnings('ignore', message='No observer defined on WCS')
+            spec = spectral_slab(self._selected_cube, spec_min, spec_max)
+            # Spatial-spatial image only.
+            collapsed_spec = spec.collapse(self.selected_func.lower(), axis=-1).T  # Quantity


Note: Transpose here fixes the orientation problem that @javerbukh found.

xref astropy/specutils#921

pllim · 2022-02-09T21:21:14Z

jdaviz/configs/default/plugins/collapse/tests/test_collapse.py


 from jdaviz.configs.default.plugins.collapse.collapse import Collapse


 @pytest.mark.filterwarnings('ignore')
-def test_linking_after_collapse(spectral_cube_wcs):
+def test_linking_after_collapse(cubeviz_helper, spectral_cube_wcs):


Note: Might as well use Cubeviz here to be more realistic.

pllim · 2022-02-09T21:22:12Z

jdaviz/configs/default/plugins/collapse/tests/test_collapse.py


-    coll.selected_data_item = 'test'
+    # This is automatically called in notebook but not in this test.


I don't know if this is frontend weirdness in unit test or a bug. Seems to work find in the plugin but please double check!

Is the intention here to update the populated spectral bounds? I think that's scheduled to be removed in the future anyways, but I am seeing it not being populated at start in this PR (but populated at start in main)...

Correct. Because it is my impression that this would automatically be picked up by @observe. So the way I refactored it, setting, self.selected_data_item actually calls self.selected_subset at the end, which I thought would kick off self._on_subset_selected() automatically to populate the bounds. Am I misunderstanding how @observe works?

Yes, except the default values set when creating the traitlets don't trigger those. Your setup should work if you have the traitlet default to an empty string instead of "None" and let it update to "None" (and the bounds populate) when the data is loaded.

I wasn't aware of that subtlety. I pushed a commit. Please see if that's correct. Thanks!

pllim · 2022-02-09T21:23:02Z

setup.cfg

    ignore::DeprecationWarning:glue
    ignore::DeprecationWarning:bqplot
    ignore::DeprecationWarning:bqplot_image_gl
    ignore::DeprecationWarning:bqscales
    ignore::DeprecationWarning:traittypes
+    ignore::DeprecationWarning:voila


I see a new warning locally, so I need this line so the tests would run on my machine.

and refactor Collapse plugin to work with specutils. Remove axis option from Collapse plugin. Change Collapse function default to sum from mean. Minor code clean-ups. Updated test and doc.

jdaviz/configs/default/plugins/collapse/collapse.py

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

javerbukh

Looks good! I'm still seeing that bug you mentioned of not being able to set the contrast and bias options for the collapsed data, but I think that's outside the scope of this PR. Nice work!

kecnry

Everything still seems to work (and am glad to be rid of the confusing axis options)! I think all of the remaining comments existed before and/or can be addressed later.

kecnry · 2022-02-10T14:41:43Z

jdaviz/configs/default/plugins/collapse/collapse.py

-    selected_func = Unicode('Mean').tag(sync=True)
+    selected_func = Unicode('Sum').tag(sync=True)


All in favor of this change... but just a note that the default "function" for the spectral-viewer is still "Maximum" (that change is out-of-scope - I would have objected to a change here if this had been Max as well, but it was already different so 🤷‍♂️ )

pllim · 2022-02-10T16:05:02Z

Thanks for all the great reviews!

pllim added this to the 2.2 milestone Dec 14, 2021

pllim requested review from duytnguyendtn, ibusko, javerbukh, kecnry, ojustino and rosteen as code owners December 14, 2021 23:38

pllim marked this pull request as draft December 14, 2021 23:38

github-actions bot added documentation Explanation of code and concepts testing labels Dec 14, 2021

pllim mentioned this pull request Dec 14, 2021

Remove dependency on spectral-cube and parse ASDF/GWCS for JWST s3d cube #1003

Closed

9 tasks

pllim added the cubeviz label Dec 14, 2021

pllim commented Dec 14, 2021

View reviewed changes

jdaviz/configs/default/plugins/collapse/collapse.py Outdated Show resolved Hide resolved

pllim commented Dec 14, 2021

View reviewed changes

pllim mentioned this pull request Dec 16, 2021

spectral_slab does not handle frequency limits astropy/specutils#909

Closed

rosteen modified the milestones: 2.2, 2.3 Dec 23, 2021

This was referenced Jan 26, 2022

Fix bug in translating 3D Spectrum1D with only spectral axis defined glue-viz/glue-astronomy#61

Closed

Cubeviz parser: Refactor parser, add ndarray support, metadata viewer #1040

Closed

pllim force-pushed the no-spectral-cube-no-cry branch from 02498bf to 511da6d Compare February 1, 2022 02:03

pllim mentioned this pull request Feb 1, 2022

Make spectral-cube optional dependency glue-viz/glue-astronomy#62

Open

pllim commented Feb 1, 2022

View reviewed changes

pllim marked this pull request as ready for review February 1, 2022 22:31

pllim force-pushed the no-spectral-cube-no-cry branch from 519ab0f to 54d04b4 Compare February 3, 2022 21:51

kecnry reviewed Feb 8, 2022

View reviewed changes

jdaviz/configs/default/plugins/collapse/collapse.vue Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

pllim force-pushed the no-spectral-cube-no-cry branch from 8c24245 to 96ed69e Compare February 9, 2022 21:15

pllim commented Feb 9, 2022

View reviewed changes

Remove dependency on spectral-cube

762bb87

and refactor Collapse plugin to work with specutils. Remove axis option from Collapse plugin. Change Collapse function default to sum from mean. Minor code clean-ups. Updated test and doc.

pllim force-pushed the no-spectral-cube-no-cry branch from 96ed69e to 762bb87 Compare February 9, 2022 21:27

kecnry reviewed Feb 9, 2022

View reviewed changes

jdaviz/configs/default/plugins/collapse/collapse.py Show resolved Hide resolved

pllim mentioned this pull request Feb 9, 2022

Collapsed shape from cube is very confusing astropy/specutils#921

Open

Let traitlet auto-update

dcbe93d

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

javerbukh approved these changes Feb 10, 2022

View reviewed changes

javerbukh added the Ready for final review label Feb 10, 2022

kecnry approved these changes Feb 10, 2022

View reviewed changes

pllim merged commit ca6df0a into spacetelescope:main Feb 10, 2022

pllim deleted the no-spectral-cube-no-cry branch February 10, 2022 16:04

Jdaviz-Triage-Bot mentioned this pull request Mar 15, 2022

Find a better way to handle linking #1171

Closed

pllim mentioned this pull request Apr 25, 2022

[BUG] Cubeviz: Collapsing on data with WCS-TAB crashes Cubeviz #1271

Closed

stscijgbot-jwql mentioned this pull request Jul 9, 2024

Remove SpectralCube (Was: Build collapse method for Spectrum1D objects) #922

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependency on spectral-cube #1006

Remove dependency on spectral-cube #1006

pllim commented Dec 14, 2021 •

edited

Loading

pllim Dec 14, 2021

rosteen Dec 15, 2021

pllim Dec 16, 2021

pllim Feb 1, 2022

rosteen Feb 1, 2022

pllim Feb 1, 2022

rosteen commented Dec 20, 2021

codecov bot commented Feb 1, 2022 •

edited

Loading

pllim Feb 1, 2022

pllim Feb 1, 2022

pllim Feb 1, 2022

pllim Feb 1, 2022

kecnry Feb 10, 2022

pllim Feb 10, 2022

pllim Feb 1, 2022

pllim Feb 1, 2022

javerbukh commented Feb 8, 2022

This comment was marked as resolved.

kecnry commented Feb 9, 2022

pllim Feb 9, 2022

pllim Feb 9, 2022 •

edited

Loading

pllim Feb 9, 2022

pllim Feb 9, 2022

kecnry Feb 9, 2022

pllim Feb 9, 2022

kecnry Feb 9, 2022

pllim Feb 9, 2022

pllim Feb 9, 2022

javerbukh left a comment

kecnry left a comment

kecnry Feb 10, 2022

pllim commented Feb 10, 2022

		@@ -16,28 +14,15 @@
		__all__ = ['Collapse']


		spaxel = u.def_unit('spaxel', 1 * u.Unit(""))


		coll.selected_data_item = 'test'
		# This is automatically called in notebook but not in this test.

		selected_func = Unicode('Mean').tag(sync=True)
		selected_func = Unicode('Sum').tag(sync=True)

Remove dependency on spectral-cube #1006

Remove dependency on spectral-cube #1006

Conversation

pllim commented Dec 14, 2021 • edited 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

rosteen commented Dec 20, 2021

codecov bot commented Feb 1, 2022 • edited Loading

Codecov Report

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

javerbukh commented Feb 8, 2022

This comment was marked as resolved.

kecnry commented Feb 9, 2022

Choose a reason for hiding this comment

pllim Feb 9, 2022 • edited Loading

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

javerbukh left a comment

Choose a reason for hiding this comment

kecnry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Feb 10, 2022

pllim commented Dec 14, 2021 •

edited

Loading

codecov bot commented Feb 1, 2022 •

edited

Loading

pllim Feb 9, 2022 •

edited

Loading