fix: buggy 1D/2D spectrum viewer x-axis links #852

mariobuikhuizen · 2021-09-09T12:20:23Z

Fixes #802

Description

This pull request is to address ...

Fixes #

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

codecov · 2021-09-09T12:25:15Z

Codecov Report

Merging #852 (ca1e58c) into main (36605e8) will increase coverage by 0.23%.
The diff coverage is 75.26%.

@@            Coverage Diff             @@
##             main     #852      +/-   ##
==========================================
+ Coverage   68.29%   68.53%   +0.23%     
==========================================
  Files          65       65              
  Lines        4741     4777      +36     
==========================================
+ Hits         3238     3274      +36     
  Misses       1503     1503

Impacted Files	Coverage Δ
jdaviz/configs/mosviz/helper.py	`72.91% <73.86%> (+1.55%)`	⬆️
jdaviz/configs/mosviz/plugins/viewers.py	`73.78% <100.00%> (+0.51%)`	⬆️
...configs/default/plugins/data_tools/file_chooser.py	`69.14% <0.00%> (+3.42%)`	⬆️

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 36605e8...ca1e58c. Read the comment docs.

mariobuikhuizen · 2021-09-09T13:41:19Z

Hmm.. I see now that my workaround for the flipped data does not work as expected.

rosteen · 2021-09-09T16:09:05Z

This is probably what you're talking about in regards to the flipped data, but for what it's worth I now see the the 1D spectrum with an ascending wavelength axis on row click, however the 2D spectrum is still in descending order. This means that if the user interacts with the 1D spectrum first, the 2D spectrum will flip to match it, but if the user interacts with the 2D spectrum first, the 1D spectrum will flip back to descending wavelength to match the 2D one. So this almost works, just needs a tweak to the initial loading of the 2D spectrum I think.

Other than that, this PR looks great. The pan/zoom warning behaves much better now.

mariobuikhuizen · 2021-09-09T16:16:57Z

Indeed. I think I can flip the 1d x-axis on row click.

rosteen · 2021-09-09T16:19:07Z

You mean 2D right? 😆

jdaviz/configs/mosviz/helper.py

pllim · 2021-09-13T13:59:55Z

jdaviz/configs/mosviz/helper.py

+            self.app.hub.broadcast(msg)
+
+    def _is_world_flipped(self):
+        spec1d = self.app.get_viewer('table-viewer')._selected_data["spectrum-viewer"]


This causes KeyError.

It's failing in the tests because the test is lazy and doesn't populate the 1D spectrum viewer for the 2D test, I think. Not sure if that's a realistic use case...I would assume we always have a 1D spectrum if we have a 2D in Mosviz.

Fix the test or make this fail silently? Prefer the former...

I missed this conversation, I changed the code to guard for this.

mosviz should support loading just one type of spectrum, as we can infer from the existence of methods such as load_1d_spectra.

jdaviz/configs/mosviz/helper.py

pllim

I don't grok the internals but here is a general review.

jdaviz/configs/mosviz/helper.py

rosteen

Looks good to me, this is a huge improvement on the current performance. Thanks!

pllim · 2021-09-14T18:57:04Z

Does this need a rebase so this can be tested on top of #733 ?

mariobuikhuizen · 2021-09-14T19:19:12Z

I've rebased on #733, and some tests now produce errors.

pllim · 2021-09-14T19:23:41Z

Re: Errors -- We won't see it here until you push out the rebased branch... 😬

mariobuikhuizen · 2021-09-14T19:27:26Z

I was trying to see if I could quickly fix it 😃, but will push it now.

pllim · 2021-09-14T19:35:53Z

Hmm looks like everything is passing...

pllim · 2021-09-14T19:38:06Z

jdaviz/configs/mosviz/plugins/slit_overlay/slit_overlay.py

@@ -78,11 +77,11 @@ def place_slit_overlay(self, *args, **kwargs):
        self.remove_slit_overlay()

        # Get data from relevant viewers
-        image_data = self.app.get_viewer("image-viewer").data()
+        image_data = self.app.get_viewer("image-viewer").state.reference_data


Hmm. This is from a different PR!

pllim · 2021-09-14T19:38:59Z

jdaviz/configs/mosviz/helper.py

+        # Warn the user if they've panned far enough away from the data
+        # that the viewers will desync
+        # Note: Because of the flipped data workaround, idx_min can be > idx_max
+        if not (0 <= idx_min < len(world) and 0 <= idx_max < len(world)):


It bothers me a bit that len(world) is calculated twice in the same line, but I am being too nitpicky, perhaps...

I don't mind changing that.

mariobuikhuizen · 2021-09-14T19:41:35Z

Strange...

Also, I see some changes after the rebase that shouldn't be there. I see now that's because the merge target is main.

mariobuikhuizen · 2021-09-14T19:42:18Z

Will rebase on main, since 733 is merged

Fixes spacetelescope#802

pllim · 2021-09-14T20:05:56Z

History and code look good now. Thanks!

rosteen · 2021-09-15T14:08:34Z

That sounded like an approval to me. Merging!

mariobuikhuizen requested review from duytnguyendtn, ibusko, javerbukh, ojustino, pllim and rosteen as code owners September 9, 2021 12:20

github-actions bot added the mosviz label Sep 9, 2021

pllim reviewed Sep 13, 2021

View reviewed changes

jdaviz/configs/mosviz/helper.py Show resolved Hide resolved

pllim reviewed Sep 13, 2021

View reviewed changes

mariobuikhuizen force-pushed the fix_802 branch from 84bc44b to ad4d502 Compare September 13, 2021 14:28

pllim reviewed Sep 13, 2021

View reviewed changes

jdaviz/configs/mosviz/helper.py Outdated Show resolved Hide resolved

pllim reviewed Sep 13, 2021

View reviewed changes

rosteen approved these changes Sep 14, 2021

View reviewed changes

mariobuikhuizen force-pushed the fix_802 branch from 7888b66 to 0f3f8dd Compare September 14, 2021 19:28

pllim reviewed Sep 14, 2021

View reviewed changes

mariobuikhuizen added 2 commits September 14, 2021 21:42

fix: buggy 1D/2D spectrum viewer x-axis links

8f7f099

Fixes spacetelescope#802

fix: flip 2d x axis on selection when world data is flipped

ab72cbf

mariobuikhuizen force-pushed the fix_802 branch from 0f3f8dd to a7759e6 Compare September 14, 2021 19:43

refactor: changes for pllims review

ca1e58c

mariobuikhuizen force-pushed the fix_802 branch from a7759e6 to ca1e58c Compare September 14, 2021 20:02

pllim added the Ready for final review label Sep 14, 2021

rosteen merged commit 75e727a into spacetelescope:main Sep 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: buggy 1D/2D spectrum viewer x-axis links #852

fix: buggy 1D/2D spectrum viewer x-axis links #852

mariobuikhuizen commented Sep 9, 2021

codecov bot commented Sep 9, 2021 •

edited

Loading

mariobuikhuizen commented Sep 9, 2021

rosteen commented Sep 9, 2021 •

edited

Loading

mariobuikhuizen commented Sep 9, 2021

rosteen commented Sep 9, 2021

pllim Sep 13, 2021

rosteen Sep 13, 2021

pllim Sep 13, 2021

mariobuikhuizen Sep 13, 2021

ibusko Sep 13, 2021

pllim left a comment

rosteen left a comment

pllim commented Sep 14, 2021

mariobuikhuizen commented Sep 14, 2021

pllim commented Sep 14, 2021

mariobuikhuizen commented Sep 14, 2021

pllim commented Sep 14, 2021

pllim Sep 14, 2021

pllim Sep 14, 2021

mariobuikhuizen Sep 14, 2021

mariobuikhuizen commented Sep 14, 2021

mariobuikhuizen commented Sep 14, 2021

pllim commented Sep 14, 2021

rosteen commented Sep 15, 2021

fix: buggy 1D/2D spectrum viewer x-axis links #852

fix: buggy 1D/2D spectrum viewer x-axis links #852

Conversation

mariobuikhuizen commented Sep 9, 2021

Description

Checklist for package maintainer(s)

codecov bot commented Sep 9, 2021 • edited Loading

Codecov Report

mariobuikhuizen commented Sep 9, 2021

rosteen commented Sep 9, 2021 • edited Loading

mariobuikhuizen commented Sep 9, 2021

rosteen commented Sep 9, 2021

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

rosteen left a comment

Choose a reason for hiding this comment

pllim commented Sep 14, 2021

mariobuikhuizen commented Sep 14, 2021

pllim commented Sep 14, 2021

mariobuikhuizen commented Sep 14, 2021

pllim commented Sep 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariobuikhuizen commented Sep 14, 2021

mariobuikhuizen commented Sep 14, 2021

pllim commented Sep 14, 2021

rosteen commented Sep 15, 2021

codecov bot commented Sep 9, 2021 •

edited

Loading

rosteen commented Sep 9, 2021 •

edited

Loading