Update Cubeviz to work on data with spectral axis last #1174

rosteen · 2022-03-16T18:27:41Z

Description

This implements compatibility with glue-viz/glue-astronomy#68, which stops reordering Cube axes and leaves things the way they are in the specutils.Spectrum1D object. Opening as draft since this is very experimental and still needs work - I've fixed the data loading and slice plugin, but I'm sure a couple other things were broken by this. Mainly I wanted other people to see it if they're curious.

TODO

Uncomment

jdaviz/tox.ini

Line 57 in a9d2dfa

#devdeps: git+https://github.com/glue-viz/glue-astronomy.git
Change

jdaviz/setup.cfg

Line 36 in a9d2dfa

glue-astronomy>=0.3.3,<0.4

to be glue-astronomy>=0.4.

xref #1190

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-03-16T18:29:23Z

The other motivation for posting this now is to make sure it doesn't conflict too much with #1040 . And it obviously needs rebasing.

pllim · 2022-03-17T12:40:36Z

jdaviz/configs/cubeviz/plugins/parsers.py

@@ -119,15 +119,24 @@ def _parse_hdu(app, hdulist, file_name=None):
        if hdu.data.dtype in (int, np.uint, np.uint32) or \
                any(x in hdu.name.lower() for x in EXT_TYPES['mask']):
            app.add_data_to_viewer('mask-viewer', data_label)
+            viewer = app.get_viewer('mask-viewer')
+            viewer.state.x_att_world = app.data_collection[data_label].id['Right Ascension']


Does this work with data with no WCS? I am trying to think how this would fit into #1040 .

I think I need a case here to fall back on Pixel Axis 0 [z] (or whichever is appropriate) for things with no WCS.

I am not sure which axis that would be. I have found that specutils only attempt to swapaxes if it detects a WCS, so for anything without WCS, I needed to transpose. If you look at PR 1040, you will see a lot of array.T happening here and there. 🤪 Do I still need that with this patch?

rosteen · 2022-03-22T14:00:41Z

CI won't pass yet because this needs glue-viz/glue-astronomy#68. The tests pass locally on my machine with that installed.

pllim · 2022-03-22T20:41:16Z

Upstream PR is merged, so theoretically, if you pin glue-astronomy to dev version in setup.cfg, your CI would pass. I also added a TODO section in your original post above because your PR should undo two things added in #1190 . FYI.

codecov · 2022-03-22T20:42:19Z

Codecov Report

Merging #1174 (8955666) into main (ac42df8) will increase coverage by 1.60%.
The diff coverage is 86.27%.

@@            Coverage Diff             @@
##             main    #1174      +/-   ##
==========================================
+ Coverage   77.93%   79.53%   +1.60%     
==========================================
  Files          90       90              
  Lines        7180     7209      +29     
==========================================
+ Hits         5596     5734     +138     
+ Misses       1584     1475     -109

Impacted Files	Coverage Δ
...igs/default/plugins/model_fitting/model_fitting.py	`34.65% <0.00%> (+0.50%)`	⬆️
...s/default/plugins/model_fitting/fitting_backend.py	`77.90% <66.66%> (ø)`
jdaviz/configs/cubeviz/plugins/slice/slice.py	`75.86% <90.90%> (-0.33%)`	⬇️
jdaviz/configs/cubeviz/helper.py	`87.23% <93.33%> (+2.38%)`	⬆️
jdaviz/app.py	`91.14% <100.00%> (+0.08%)`	⬆️
jdaviz/configs/cubeviz/plugins/parsers.py	`56.93% <100.00%> (+1.13%)`	⬆️
jdaviz/configs/cubeviz/plugins/viewers.py	`98.07% <100.00%> (+0.51%)`	⬆️
...daviz/configs/default/plugins/collapse/collapse.py	`96.07% <100.00%> (ø)`
...default/plugins/gaussian_smooth/gaussian_smooth.py	`86.48% <100.00%> (+0.18%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 317e03d...8955666. Read the comment docs.

rosteen · 2022-04-05T21:57:07Z

I think this is almost ready for in-depth review. There's one persistent bug that I'm aware of that I haven't fixed yet, where calculating a moment with a viewer selected to auto-plot the results will throw an IncompatibleDataException error from Glue compute_fixed_resolution_buffer and crash the app (note that it works fine if you calculate the moment, then go into the data selection menu for the viewer and change to the moment data there). I need to log off for the day, I wonder if you might have some insight on that @astrofrog? To reproduce, run the CubevizExample notebook and go to the Moment Maps plugin, select a viewer (I recommend Center or Right) and hit Calculate. Reminder that this PR requires dev glue-astronomy.

rosteen · 2022-04-06T13:03:52Z

I fixed the above error with a small change to app.py to clear existing data in a viewer first before selecting the new data, rather than doing both in one step.

pllim · 2022-04-06T13:43:25Z

We need to pin to glue-astronomy dev with this PR, right?

rosteen · 2022-04-06T13:44:39Z

We need to pin to glue-astronomy dev with this PR, right?

Yes, the setup.cfg file here has it pinned.

astrofrog · 2022-04-06T15:45:39Z

@rosteen - did you fix the issue you asked me about above or should I still investigate?

rosteen · 2022-04-06T15:48:13Z

@rosteen - did you fix the issue you asked me about above or should I still investigate?

I did figure out a fix, no need to investigate.

pllim · 2022-04-06T16:09:30Z

jdaviz/configs/cubeviz/plugins/viewers.py

+        # Make sure that the x_att is correct on data load
+        ref_data = self.state.reference_data
+        if ref_data and ref_data.ndim == 3:
+            for att_name in ["Right Ascension", "RA"]:


Is there a better way to detect world vs pixel? I feel like hardcoding RA might box us in to only certain world coordinate frames.

Digging into Glue and looking at what the other options that might occur here are, it seems like Galactic Longitude/Latitude are probably the only others we might need to worry about (I don't want to mess with supporting solar people and their crazy helioprojective things right now 😆). I don't know if people would want lat or lon on the x-axis, I'll ask the POs.

pllim · 2022-04-06T16:12:37Z

jdaviz/configs/cubeviz/helper.py

+        viewer = self.app.get_viewer("spectrum-viewer")
+        ref_data = viewer.state.reference_data
+        if ref_data and ref_data.ndim == 3:
+            for att_name in ["Wave", "Wavelength", "Freq", "Frequency"]:


Do people make cubes in wave number, etc? How do these map to supported CTYPE and how many of those we need to actually support?

https://github.com/astropy/astropy/blob/43ee5806e9c6f7d58c12c1cb9287b3c61abe489d/astropy/wcs/wcsapi/fitswcs.py#L88

Looking at the glue-astronomy translator, it seems like the other likely options are Wavenumber, Velocity and Energy. I'll add those.

…fter initial data load.

…ace condition

javerbukh · 2022-04-12T13:15:09Z

I can get more specific about how to reproduce:

Run cubeviz example notebook
Create a spatial subset and a spectral subset
Create a model that is a first order polynomial
Fit model, show in middle viewer, and apply to cube
Open line analysis and get traceback

Cubeviz still runs but the line analysis plugin hangs.

javerbukh · 2022-04-12T13:30:08Z

Looks like everything works great other than the error I mentioned and the spectrum viewer not automatically reseting limits. If the former originates outside the PR and the latter will work after merge/rebase, I'm ready to approve this.

rosteen · 2022-04-12T13:34:18Z

I'm investigating the cube fitting, it is a problem with this PR and I should have a fix soon (I hope). This also led me to notice that the fit to the spectrum isn't getting properly plotted in the spectrum viewer either, so I'm trying to figure that out as well. Good catch.

rosteen · 2022-04-12T16:08:49Z

@javerbukh I think that the last bug in the model fitting is also present on main, could you confirm that I'm not crazy? Basically if you fit the whole cube with a spectral subset selected, the results comes back with all zero values. Very not great, but not due to these changes as far as I can tell.

javerbukh · 2022-04-12T16:24:17Z

This is what I see on main. I do not see the same error as I saw on this branch.

rosteen · 2022-04-12T16:25:27Z

What does this give you?

fit = viz.app.data_collection["Model [Cube] 1"]
fit['flux'].max()

javerbukh · 2022-04-12T16:26:45Z

0.0

jdaviz/configs/cubeviz/plugins/slice/slice.py

jdaviz/configs/default/plugins/gaussian_smooth/gaussian_smooth.py

rosteen · 2022-04-12T20:31:16Z

See #1245 for the followup on the Cubeviz fitting bug.

kecnry

Everything seems to work now, and it'll definitely be good to get this change in and behind us!

javerbukh · 2022-04-13T13:11:30Z

When I press Apply to Cube in model fitting I do not see any indication that jdaviz is working on it (loading bar or the spinning thing).

rosteen · 2022-04-13T14:01:26Z

When I press Apply to Cube in model fitting I do not see any indication that jdaviz is working on it (loading bar or the spinning thing).

This is also present on main. I think it happens when a previous snackbar message is still up when the "Apply to Cube" button is clicked. If you wait/clear all the other snackbar messages first it appears. I'll make an issue for this.

Edit: See #1246

javerbukh

Sounds good, thanks for opening the ticket!

pllim added the cubeviz label Mar 16, 2022

This comment was marked as resolved.

# to view

rosteen added the Upstream fix required label Mar 16, 2022

rosteen force-pushed the cubeviz-noodling branch from 9191040 to 21d8488 Compare March 16, 2022 18:45

pllim reviewed Mar 17, 2022

View reviewed changes

rosteen mentioned this pull request Mar 17, 2022

Generalize PaddedSpectrumWCS for higher dimensionality glue-viz/glue-astronomy#68

Merged

rosteen force-pushed the cubeviz-noodling branch from 80a78bb to 4b2f356 Compare March 21, 2022 20:19

rosteen added this to the 2.4 milestone Mar 21, 2022

rosteen marked this pull request as ready for review March 21, 2022 21:38

rosteen requested review from duytnguyendtn, javerbukh, ibusko, ojustino and kecnry as code owners March 21, 2022 21:38

pllim modified the milestones: 2.4, 2.5 Mar 29, 2022

rosteen force-pushed the cubeviz-noodling branch 2 times, most recently from 6ec3102 to 6e8250f Compare April 5, 2022 21:18

pllim reviewed Apr 6, 2022

View reviewed changes

rosteen added 8 commits April 12, 2022 09:14

Move logic to viewer callback to avoid duplication and also trigger a…

72dbca5

…fter initial data load.

Spectrum viewer axis logic can't be a callback in the viewer due to r…

e550904

…ace condition

Fix adding result from plugin to viewer on calculation

0716f09

Remove redundant fix that was superceded by other work

f8b126c

Update subset tests to use Cubeviz helper instead of base Application

5b59272

Fix codestyle and failing test on empty viewer

e7bc52a

Add more potential spectral axis names

911587a

Add Galactic Longitude as option for x-axis autoset

c486d2e

Fixing model fit axis order and linking

485a20a

rosteen force-pushed the cubeviz-noodling branch from 6fd3e19 to 485a20a Compare April 12, 2022 16:07

Ditch unused imports

7978c06

kecnry reviewed Apr 12, 2022

View reviewed changes

jdaviz/configs/cubeviz/plugins/slice/slice.py Show resolved Hide resolved

kecnry reviewed Apr 12, 2022

View reviewed changes

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

rosteen added 3 commits April 12, 2022 14:55

Autoscale spectrum y axis on load properly

eb938f6

Reset limits without triggering warning

7d530d8

Uncomment glue-astronomy devdep

8955666

kecnry approved these changes Apr 13, 2022

View reviewed changes

javerbukh approved these changes Apr 13, 2022

View reviewed changes

rosteen merged commit d1a6ea1 into spacetelescope:main Apr 13, 2022

rosteen deleted the cubeviz-noodling branch April 13, 2022 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Cubeviz to work on data with spectral axis last #1174

Update Cubeviz to work on data with spectral axis last #1174

rosteen commented Mar 16, 2022 •

edited

Loading

rosteen commented Mar 16, 2022 •

edited

Loading

This comment was marked as resolved.

pllim Mar 17, 2022

rosteen Mar 17, 2022

pllim Mar 17, 2022

rosteen commented Mar 22, 2022

pllim commented Mar 22, 2022

codecov bot commented Mar 22, 2022 •

edited

Loading

rosteen commented Apr 5, 2022

rosteen commented Apr 6, 2022

pllim commented Apr 6, 2022

rosteen commented Apr 6, 2022

astrofrog commented Apr 6, 2022

rosteen commented Apr 6, 2022

pllim Apr 6, 2022

rosteen Apr 7, 2022

pllim Apr 6, 2022

rosteen Apr 7, 2022

javerbukh commented Apr 12, 2022

javerbukh commented Apr 12, 2022

rosteen commented Apr 12, 2022

rosteen commented Apr 12, 2022

javerbukh commented Apr 12, 2022

rosteen commented Apr 12, 2022

javerbukh commented Apr 12, 2022

rosteen commented Apr 12, 2022

kecnry left a comment

javerbukh commented Apr 13, 2022

rosteen commented Apr 13, 2022 •

edited

Loading

javerbukh left a comment

Update Cubeviz to work on data with spectral axis last #1174

Update Cubeviz to work on data with spectral axis last #1174

Conversation

rosteen commented Mar 16, 2022 • edited Loading

Description

TODO

Checklist for package maintainer(s)

rosteen commented Mar 16, 2022 • edited Loading

This comment was marked as resolved.

pllim Mar 17, 2022

Choose a reason for hiding this comment

rosteen Mar 17, 2022

Choose a reason for hiding this comment

pllim Mar 17, 2022

Choose a reason for hiding this comment

rosteen commented Mar 22, 2022

pllim commented Mar 22, 2022

codecov bot commented Mar 22, 2022 • edited Loading

Codecov Report

rosteen commented Apr 5, 2022

rosteen commented Apr 6, 2022

pllim commented Apr 6, 2022

rosteen commented Apr 6, 2022

astrofrog commented Apr 6, 2022

rosteen commented Apr 6, 2022

pllim Apr 6, 2022

Choose a reason for hiding this comment

rosteen Apr 7, 2022

Choose a reason for hiding this comment

pllim Apr 6, 2022

Choose a reason for hiding this comment

rosteen Apr 7, 2022

Choose a reason for hiding this comment

javerbukh commented Apr 12, 2022

javerbukh commented Apr 12, 2022

rosteen commented Apr 12, 2022

rosteen commented Apr 12, 2022

javerbukh commented Apr 12, 2022

rosteen commented Apr 12, 2022

javerbukh commented Apr 12, 2022

rosteen commented Apr 12, 2022

kecnry left a comment

Choose a reason for hiding this comment

javerbukh commented Apr 13, 2022

rosteen commented Apr 13, 2022 • edited Loading

javerbukh left a comment

Choose a reason for hiding this comment

rosteen commented Mar 16, 2022 •

edited

Loading

rosteen commented Mar 16, 2022 •

edited

Loading

codecov bot commented Mar 22, 2022 •

edited

Loading

rosteen commented Apr 13, 2022 •

edited

Loading