Cubeviz image viewer coordinates display #1315

pllim · 2022-05-12T17:29:44Z

Description

This pull request is to add Imviz-like coordinates display for Cubeviz image viewers.

This is all UI so I am not sure how to add tests. There is currently no unit test for this equivalent capability in Imviz. Reviewers should interactively load a few different datasets that we are supposed to support and see what happens...

Here is a screenshot with data from Cubeviz example notebook (the mouse was at the center of flux viewer for this one):

Fixes #1090

Imviz's display was originally done by @astrofrog (e.g., #557).

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? (N/A)
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)?

codecov · 2022-05-12T17:38:28Z

Codecov Report

Merging #1315 (4cf665c) into main (cac9374) will decrease coverage by 0.14%.
The diff coverage is 74.54%.

@@            Coverage Diff             @@
##             main    #1315      +/-   ##
==========================================
- Coverage   83.56%   83.42%   -0.15%     
==========================================
  Files          91       91              
  Lines        7576     7620      +44     
==========================================
+ Hits         6331     6357      +26     
- Misses       1245     1263      +18

Impacted Files	Coverage Δ
jdaviz/configs/cubeviz/plugins/viewers.py	`87.09% <70.73%> (-12.91%)`	⬇️
jdaviz/configs/cubeviz/plugins/parsers.py	`57.85% <85.71%> (+0.92%)`	⬆️
...configs/default/plugins/data_tools/file_chooser.py	`65.71% <0.00%> (-3.43%)`	⬇️

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 cac9374...4cf665c. Read the comment docs.

kecnry · 2022-05-13T13:53:14Z

Any chance this can be generalized (without too much effort) and moved to a Mixin that both cubeviz and imviz viewers inherit from (especially if adding the same to specviz is in our future)? It seems like a lot of the code is the same.

pllim · 2022-05-13T14:25:22Z

Re: Refactoring -- I'd rather wait to see what needs to happen for #1299 first.

kecnry · 2022-05-13T16:45:08Z

Ok, do you want to move ahead with reviews on this as-is (ignoring refactoring) then and deal with that as a separate effort? Or should we put this PR on hold until 1299 is resolved?

pllim · 2022-05-13T16:46:10Z

do you want to move ahead with reviews

I'd prefer we move ahead as-is. Thanks!

kecnry

I'd prefer we move ahead as-is.

Ok, then let's refactor if/when we add to any other viz or when 1299 is resolved. I don't love the hit to coverage, but the interactive nature is difficult to test without more UI testing infrastructure. At least this is something that will be very obvious if it completely breaks (although very non-obvious if it just displays the wrong information). Is it possible to fake the data sent to on_mouse_or_key_event to at least make sure the value and a single known pixel is parsed correctly?

kecnry · 2022-05-16T15:52:02Z

jdaviz/configs/cubeviz/plugins/viewers.py

+            # Extract first dataset from visible layers and use this for coordinates - the choice
+            # of dataset shouldn't matter if the datasets are linked correctly
+            image = visible_layers[0].layer


how is visible_layers ordered? If there are multiple layers and they're linked correctly, the coordinates shouldn't care what layer, but the value will... is this guaranteed to be the "top" layer if multiple layers are plotted without changing transparency? Is there any room to squeeze in the layer name for cases where visible_layers includes more than one?

This behavior/situation might be something that is worth adding test coverage, if possible.

The inner workings of visible layers, I'd have to defer to @astrofrog or @maartenbreddels. I wish there is an easier way for me to reliably grab this info too but then Glue lets you overlay this and that, which complicates things. This particular info panel would ignore the semi-transparent overlay use cases.

Re: tests -- the info panel is kinda tested non-interactively in https://github.com/spacetelescope/jdaviz/blob/main/jdaviz/configs/imviz/tests/test_linking.py (the label_mouseover stuff). Not that the tests are exhaustive because they obviously missed #1299 but again adding a test for that would be out of scope here. Still, I guess I can add similar test(s) to Cubeviz...

from trial and error, it seems that this does always show the "top" layer, which is the most sensible (simple) behavior. If there is any room, I do think a label would be useful, but will approve this and that can always be added later if you don't have time to include it in scope here (as I think the same thing could apply to the existing display in imviz).

Re: label -- In Imviz, I incorporated that into Compass plugin (because the label just seems to go with Compass naturally). Originally when I was new and naive, I attempted glue-viz/glue-jupyter#244 but failed. I don't think there is any more space for label in the coordinates display, as it is already pretty crowded.

This does bring up a good point... What if someone deselects the cube and have a 2D moment map displayed in the cube viewer instead? Is that a use case we even support?

for Spectrum1D in Cubeviz. TST: Add Cubeviz coords info tests. DOC: Update change log.

pllim

@kecnry , I added tests but might also have opened a small rabbit hole. Please re-review. Thanks!

pllim · 2022-05-16T21:15:14Z

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

-    app.add_data(spectrum1d_cube, 'test[FLUX]')
-    app.add_data_to_viewer('flux-viewer', 'test[FLUX]')
+    dc = cubeviz_helper.app.data_collection
+    cubeviz_helper.load_data(spectrum1d_cube, data_label='test')


Since I am updating this test anyway, I took the liberty to make it more realistic in loading like a user would.

pllim · 2022-05-16T21:16:34Z

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

@@ -30,11 +28,20 @@ def test_moment_calculation(cubeviz_helper, spectrum1d_cube, tmpdir):
    assert mm.results_label_overwrite is True

    result = dc[1].get_object(cls=CCDData)
-    assert result.shape == (2, 4)  # Cube shape is (2, 2, 4)
+    assert result.shape == (4, 2)  # Cube shape is (2, 2, 4)


But in changing from using app.add_data to cubeviz_helper.load_data, the moment map shape changed! @rosteen , since you refactored how Cubeviz swaps dimensions around, does this make sense to you?

pllim · 2022-05-16T21:17:47Z

jdaviz/configs/cubeviz/plugins/parsers.py

@@ -75,9 +75,9 @@ def parse_data(app, file_obj, data_type=None, data_label=None):
    #  into something glue can understand.
    elif isinstance(file_obj, Spectrum1D):
        if file_obj.flux.ndim == 3:
-            _parse_spectrum1d_3d(app, file_obj)
+            _parse_spectrum1d_3d(app, file_obj, data_label=data_label)


It is highly annoying to me that data_label is ignored when you pass Spectrum1D into Cubeviz, so I fixed it here. Unless there is a good reason not to do this? I cannot think of any good reason.

Thank you for doing this! It's actually bothered me before but I guess not enough to add it 😅

javerbukh

Looks good!

pllim added the 💤 enhancement New feature or request label May 12, 2022

pllim added this to the 2.6 milestone May 12, 2022

pllim requested review from duytnguyendtn, rosteen, javerbukh, ojustino and kecnry as code owners May 12, 2022 17:29

github-actions bot added cubeviz documentation Explanation of code and concepts labels May 12, 2022

javerbukh mentioned this pull request May 13, 2022

Add tool that shows spectrum at spaxel in cubeviz #1317

Merged

15 tasks

kecnry reviewed May 16, 2022

View reviewed changes

kecnry approved these changes May 16, 2022

View reviewed changes

kecnry added the Ready for final review label May 16, 2022

pllim added 2 commits May 16, 2022 15:39

Cubeviz image viewer coordinates display

5b3e32d

BUG: Fixed data_label not propagated

4cf665c

for Spectrum1D in Cubeviz. TST: Add Cubeviz coords info tests. DOC: Update change log.

pllim force-pushed the i-viz-dead-pixels branch from 649bca8 to 4cf665c Compare May 16, 2022 21:13

pllim commented May 16, 2022

View reviewed changes

pllim mentioned this pull request May 17, 2022

Data Dropdown Component #1313

Merged

13 tasks

javerbukh mentioned this pull request May 17, 2022

plotting moment map on top of cube fails #1323

Closed

javerbukh approved these changes May 17, 2022

View reviewed changes

javerbukh merged commit 926623e into spacetelescope:main May 17, 2022

pllim deleted the i-viz-dead-pixels branch May 17, 2022 14:01

pllim mentioned this pull request May 18, 2022

Allow cubeviz image viewer to display coords for 2D data #1326

Merged

9 tasks

pllim mentioned this pull request Jul 4, 2022

BUG: Cubeviz coordinate display should follow top visible layer #1445

Merged

9 tasks

stscijgbot-jwql mentioned this pull request Jul 9, 2024

Cubeviz should have coordinates display like Imviz #1090

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cubeviz image viewer coordinates display #1315

Cubeviz image viewer coordinates display #1315

pllim commented May 12, 2022 •

edited

Loading

codecov bot commented May 12, 2022 •

edited

Loading

kecnry commented May 13, 2022

pllim commented May 13, 2022

kecnry commented May 13, 2022

pllim commented May 13, 2022

kecnry left a comment

kecnry May 16, 2022

pllim May 16, 2022

kecnry May 16, 2022

pllim May 16, 2022

pllim left a comment

pllim May 16, 2022

pllim May 16, 2022

pllim May 16, 2022

javerbukh May 17, 2022

javerbukh left a comment

Cubeviz image viewer coordinates display #1315

Cubeviz image viewer coordinates display #1315

Conversation

pllim commented May 12, 2022 • edited Loading

Description

Checklist for package maintainer(s)

codecov bot commented May 12, 2022 • edited Loading

Codecov Report

kecnry commented May 13, 2022

pllim commented May 13, 2022

kecnry commented May 13, 2022

pllim commented May 13, 2022

kecnry left a 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 left a 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

pllim commented May 12, 2022 •

edited

Loading

codecov bot commented May 12, 2022 •

edited

Loading