Fix Cubeviz mouseover bugs, add WCS to moment map #2009

pllim · 2023-02-11T02:32:30Z

Description

This pull request is to:

fix BUG: Cubeviz parser gives wrong sky coordinates for non-flux cube under certain condition #1991
fix Moment Map plugin should write out moment map WCS too #1516

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

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 milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

Moment map now has WCS attached to it. Fix tests.

codecov · 2023-02-11T02:47:05Z

Codecov Report

Base: 92.09% // Head: 92.08% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (e72fd55) compared to base (5216e81).
Patch coverage: 96.42% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2009      +/-   ##
==========================================
- Coverage   92.09%   92.08%   -0.02%     
==========================================
  Files         140      140              
  Lines       15373    15381       +8     
==========================================
+ Hits        14158    14163       +5     
- Misses       1215     1218       +3

Impacted Files	Coverage Δ
jdaviz/configs/cubeviz/plugins/parsers.py	`68.46% <ø> (-0.15%)`	⬇️
...z/configs/imviz/plugins/coords_info/coords_info.py	`93.57% <90.90%> (-1.06%)`	⬇️
jdaviz/app.py	`94.42% <100.00%> (ø)`
...configs/cubeviz/plugins/moment_maps/moment_maps.py	`94.87% <100.00%> (+0.20%)`	⬆️
...eviz/plugins/moment_maps/tests/test_moment_maps.py	`100.00% <100.00%> (ø)`
...aviz/configs/cubeviz/plugins/tests/test_parsers.py	`100.00% <100.00%> (ø)`

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.

javerbukh · 2023-02-23T20:00:48Z

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

@@ -36,7 +37,7 @@ def test_moment_calculation(cubeviz_helper, spectrum1d_cube, tmpdir):
    assert len(mv_data) == 1
    assert mv_data[0].label == 'moment 0'

-    assert len(dc.links) == 8
+    assert len(dc.links) == 14


Do you know why there are so many more links? Is it because the moment map now has WCS and WCS uses more links?

That is my guess. Before this, moment map has no WCS, and we hacked Cubeviz to secretly grab the WCS from the "flux" cube.

But why 14 and not 10, that I don't know. Someone who knows how to read the links mini-language can tell me.

If you run the test case in an interactive notebook, you can get out the following. Does this look about right? Or is something off?

for i, link in enumerate(dc.links): print(f"{i+1}: " + link.to_html() + "<br/>")

1: [moment 0].Pixel Axis 1 [x] ← WCSLink.forwards_1([test[FLUX]].Pixel Axis 1 [y], [test[FLUX]].Pixel Axis 0 [z])
2: [test[FLUX]].Declination ← using([test[FLUX]].Pixel Axis 0 [z], [test[FLUX]].Pixel Axis 1 [y])
3: [test[FLUX]].Wave ← using([test[FLUX]].Pixel Axis 2 [x])
4: [moment 0].Pixel Axis 0 [y] ← using([moment 0].Declination, [moment 0].Right Ascension)
5: [moment 0].Pixel Axis 1 [x] ← using([moment 0].Declination, [moment 0].Right Ascension)
6: [test[FLUX]].Pixel Axis 0 [z] ← WCSLink.backwards_2([moment 0].Pixel Axis 1 [x], [moment 0].Pixel Axis 0 [y])
7: [test[FLUX]].Pixel Axis 0 [z] ← using([test[FLUX]].Right Ascension, [test[FLUX]].Declination)
8: [moment 0].Pixel Axis 0 [y] ← WCSLink.forwards_2([test[FLUX]].Pixel Axis 1 [y], [test[FLUX]].Pixel Axis 0 [z])
9: [moment 0].Declination ← using([moment 0].Pixel Axis 0 [y], [moment 0].Pixel Axis 1 [x])
10: [test[FLUX]].Pixel Axis 1 [y] ← using([test[FLUX]].Right Ascension, [test[FLUX]].Declination)
11: [moment 0].Right Ascension ← using([moment 0].Pixel Axis 0 [y], [moment 0].Pixel Axis 1 [x])
12: [test[FLUX]].Pixel Axis 2 [x] ← using([test[FLUX]].Wave)
13: [test[FLUX]].Pixel Axis 1 [y] ← WCSLink.backwards_1([moment 0].Pixel Axis 1 [x], [moment 0].Pixel Axis 0 [y])
14: [test[FLUX]].Right Ascension ← using([test[FLUX]].Pixel Axis 0 [z], [test[FLUX]].Pixel Axis 1 [y])

The original 8:

2: [test[FLUX]].Declination ← using([test[FLUX]].Pixel Axis 0 [z], [test[FLUX]].Pixel Axis 1 [y])
3: [test[FLUX]].Wave ← using([test[FLUX]].Pixel Axis 2 [x])
6: [test[FLUX]].Pixel Axis 0 [z] ← WCSLink.backwards_2([moment 0].Pixel Axis 1 [x], [moment 0].Pixel Axis 0 [y])
7: [test[FLUX]].Pixel Axis 0 [z] ← using([test[FLUX]].Right Ascension, [test[FLUX]].Declination)
10: [test[FLUX]].Pixel Axis 1 [y] ← using([test[FLUX]].Right Ascension, [test[FLUX]].Declination)
12: [test[FLUX]].Pixel Axis 2 [x] ← using([test[FLUX]].Wave)
13: [test[FLUX]].Pixel Axis 1 [y] ← WCSLink.backwards_1([moment 0].Pixel Axis 1 [x], [moment 0].Pixel Axis 0 [y])
14: [test[FLUX]].Right Ascension ← using([test[FLUX]].Pixel Axis 0 [z], [test[FLUX]].Pixel Axis 1 [y])

Additional 6:

1: [moment 0].Pixel Axis 1 [x] ← WCSLink.forwards_1([test[FLUX]].Pixel Axis 1 [y], [test[FLUX]].Pixel Axis 0 [z])
4: [moment 0].Pixel Axis 0 [y] ← using([moment 0].Declination, [moment 0].Right Ascension)
5: [moment 0].Pixel Axis 1 [x] ← using([moment 0].Declination, [moment 0].Right Ascension)
8: [moment 0].Pixel Axis 0 [y] ← WCSLink.forwards_2([test[FLUX]].Pixel Axis 1 [y], [test[FLUX]].Pixel Axis 0 [z])
9: [moment 0].Declination ← using([moment 0].Pixel Axis 0 [y], [moment 0].Pixel Axis 1 [x])
11: [moment 0].Right Ascension ← using([moment 0].Pixel Axis 0 [y], [moment 0].Pixel Axis 1 [x])

It looks like there are two links between the moment and flux data pixel axes, two mapping pixel to RA/Dec, and two mapping RA/Dec to pixel. Considering similar pixels <-> RA/Dec links exist for the flux data, this doesn't worry me (despite not 100% understanding all of the underlying linking logic). I'd be ok merging.

javerbukh

Tested and it works well! I'm just worried about all the changes to linking in the tests but the added functionality is worth the slightly slower load time.

kecnry

Does this same fix affect - or should be applied to - any other plugins (collapse for example)?

kecnry · 2023-02-24T14:09:21Z

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

-    # Link 3:
-    # Pixel Axis 0 [z] from cube.pixel_component_ids[0]
-    # Pixel Axis 1 [x] from plugin.pixel_component_ids[1]
-    assert dc.external_links[2].cids1[0] == dc[0].pixel_component_ids[0]
-    assert dc.external_links[2].cids2[0] == dc[-1].pixel_component_ids[1]
-    # Link 4:
-    # Pixel Axis 1 [y] from cube.pixel_component_ids[1]
-    # Pixel Axis 0 [y] from plugin.pixel_component_ids[0]
-    assert dc.external_links[3].cids1[0] == dc[0].pixel_component_ids[1]
-    assert dc.external_links[3].cids2[0] == dc[-1].pixel_component_ids[0]


should these tests be replaced with anything with the new linking, or are they irrelevant now?

Those external links do not exist anymore. There is nothing to replace.

but now there are new internal links..... do those need testing or is that testing covered by other WCS linking tests?

Please see #2009 (comment)

ok... so should we hold off merge until someone can dig into that and confirm? @javerbukh?

Probably cannot get to that today but it's a good opportunity for someone to get familiar with linking! 😉

pllim · 2023-02-24T14:18:41Z

any other plugins (collapse for example)?

Maybe spatial collapse, but then again, you cannot really write it back out in the plugin, so it does not matter for now?

kecnry

Maybe spatial collapse, but then again, you cannot really write it back out in the plugin, so it does not matter for now?

Discussed offline and decided this can be a follow-up effort (technically the user can still manually save to a file, and we may implement saving to a file in the UI in the future... so would probably still be good to update).

Since @rosteen looked at the links and no one sees any major issues... once that ticket is created and this is rebased ~~and passing CI~~ (EDIT: nevermind, just changelog in conflict, so CI should be good to go!), feel free to merge!

pllim · 2023-02-27T18:43:05Z

@kecnry , I opened a follow up issue at #2038 .

Thanks, all!

pllim · 2023-03-03T14:08:40Z

Okay, we cannot backport this because it builds on #1976.

NOTE: this is just switching one wrong value for another since spacetelescope#2009 is not being backported

…r) (#2060) * Backport PR #2056: fix initial slice of uncert-viewer * update mouseover test NOTE: this is just switching one wrong value for another since #2009 is not being backported --------- Co-authored-by: P. L. Lim <[email protected]> Co-authored-by: Kyle Conroy <[email protected]>

Fix Cubeviz mouseover bugs.

6f53583

Moment map now has WCS attached to it. Fix tests.

pllim added bug Something isn't working feature Feature request labels Feb 11, 2023

pllim added this to the 3.4 milestone Feb 11, 2023

github-actions bot added cubeviz imviz labels Feb 11, 2023

pllim mentioned this pull request Feb 11, 2023

BUG: Cubeviz parser gives wrong sky coordinates for non-flux cube under certain condition #1991

Closed

Remove outdated comment

fcdbde0

pllim marked this pull request as ready for review February 11, 2023 04:42

pllim requested review from duytnguyendtn, rosteen, javerbukh, kecnry, bmorris3 and haticekaratay as code owners February 11, 2023 04:42

javerbukh reviewed Feb 23, 2023

View reviewed changes

javerbukh approved these changes Feb 23, 2023

View reviewed changes

pllim added the Ready for final review label Feb 23, 2023

kecnry reviewed Feb 24, 2023

View reviewed changes

kecnry approved these changes Feb 27, 2023

View reviewed changes

Merge branch 'main' into show-me-the-pixels-the-fix

e72fd55

pllim merged commit 19bc691 into spacetelescope:main Feb 27, 2023

pllim deleted the show-me-the-pixels-the-fix branch February 27, 2023 18:31

Jdaviz-Triage-Bot mentioned this pull request Feb 27, 2023

Cubeviz: Collapsed 2D image should have WCS #2038

Open

This was referenced Mar 2, 2023

Allow spectral axis to be anywhere, instead of forcing it to be last astropy/specutils#999

Closed

Backport PR #2056 on branch v3.3.x (fix initial slice of uncert-viewer) #2060

Merged

This comment was marked as resolved.

Sign in to view

lumberbot-app bot added the Still Needs Manual Backport label Mar 3, 2023

pllim removed the Still Needs Manual Backport label Mar 3, 2023

kecnry added a commit to meeseeksmachine/jdaviz that referenced this pull request Mar 3, 2023

update mouseover test

3d4bca8

NOTE: this is just switching one wrong value for another since spacetelescope#2009 is not being backported

kecnry added a commit to meeseeksmachine/jdaviz that referenced this pull request Mar 3, 2023

update mouseover test

d8c6530

NOTE: this is just switching one wrong value for another since spacetelescope#2009 is not being backported

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Cubeviz mouseover bugs, add WCS to moment map #2009

Fix Cubeviz mouseover bugs, add WCS to moment map #2009

pllim commented Feb 11, 2023 •

edited

Loading

codecov bot commented Feb 11, 2023 •

edited

Loading

javerbukh Feb 23, 2023

pllim Feb 23, 2023

pllim Feb 23, 2023

pllim Feb 24, 2023

pllim Feb 24, 2023 •

edited

Loading

rosteen Feb 27, 2023

javerbukh left a comment

kecnry left a comment

kecnry Feb 24, 2023

pllim Feb 24, 2023

kecnry Feb 24, 2023

pllim Feb 24, 2023

kecnry Feb 24, 2023

javerbukh Feb 24, 2023

pllim commented Feb 24, 2023

kecnry left a comment •

edited

Loading

pllim commented Feb 27, 2023

This comment was marked as resolved.

This comment was marked as resolved.

pllim commented Mar 3, 2023

Fix Cubeviz mouseover bugs, add WCS to moment map #2009

Fix Cubeviz mouseover bugs, add WCS to moment map #2009

Conversation

pllim commented Feb 11, 2023 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Feb 11, 2023 • 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

pllim Feb 24, 2023 • edited Loading

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

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 Feb 24, 2023

kecnry left a comment • edited Loading

Choose a reason for hiding this comment

pllim commented Feb 27, 2023

This comment was marked as resolved.

This comment was marked as resolved.

pllim commented Mar 3, 2023

pllim commented Feb 11, 2023 •

edited

Loading

codecov bot commented Feb 11, 2023 •

edited

Loading

pllim Feb 24, 2023 •

edited

Loading

kecnry left a comment •

edited

Loading