WIP: Fix blockers #22

pllim · 2023-11-15T00:03:30Z

What I had to do to make the API work again for this fake WCS layer but I don't love this. Probably have to special case everything just for Imviz so the old API behavior would still work for Cubeviz and Mosviz image viewers.

TODO

Check CI.
Read diff and apply more review as changes.
Re-test various workflows (different linkings, multi-viewer, multi-rotation, compass of rotated data, aper phot, etc).
Other vizes?

🐱

pllim · 2023-11-16T00:15:48Z

jdaviz/configs/imviz/plugins/viewers.py

            self.state.y_max
        ]

-        y_corners, x_corners = self._get_real_xy(


BTW I thought this was clever but unfortunately, _get_real_xy was never meant to be vectorized. I dunno why but passing in scalar x/y in gives different output than passing in lists. Passing in scalars is what we have been using in main and the results are correct.

jdaviz/configs/imviz/plugins/viewers.py

pllim · 2023-11-17T05:20:53Z

jdaviz/configs/imviz/tests/test_linking.py

I have to make a lot of changes here because I think there was a misunderstanding on the subtleties. And also some of the tests no longer make sense now that you always have a fake GWCS with no bounding box as reference.

pllim · 2023-11-17T05:22:22Z

jdaviz/configs/imviz/tests/test_simple_aper_phot.py

No results should have changed from main. The fact that you changed the results in your branch was a red flag. I managed to adjust the test inputs that do not result in results changing.

codecov-commenter · 2023-11-17T05:32:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (wcs-only-layers@e95aefa). Click here to learn what that means.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                Coverage Diff                 @@
##             wcs-only-layers      #22   +/-   ##
==================================================
  Coverage                   ?   90.09%           
==================================================
  Files                      ?      160           
  Lines                      ?    19852           
  Branches                   ?        0           
==================================================
  Hits                       ?    17885           
  Misses                     ?     1967           
  Partials                   ?        0

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

jdaviz/configs/imviz/helper.py

More safeguards fix multi viewer linked pan/zoom (or breaking it, not sure) Fix CI mainly by using more conventional ways to adjust the tests take give back same result as main, so also undo the result changes. Deleted unnecessary functions. Better change log, add basic doc. It bothers me that there is no user doc at all. I will add some basic info here. Better doc is already a future ticket. I do not need get_bottom_layer is needed GWCS is also BaseHighLevelWCS, no need to check separately. imviz.link_data API should error out sooner when subsets need to be cleared first. Remove unnecessary check for viewer.data() especially since that might waste memory. More code simplification. And clarify behavior more in doc. Docstring nitpick Nope did not work out as I hoped I think I fixed aper phot

because there is GUI

pllim · 2023-11-29T18:20:24Z

Calls to default_viewer have to be updated (again) when we rebase on upstream/main. Hopefully a simple default_viewer._obj would do.

Do not assume data_collection[0] is always real data.

Fix linking in batch mode. Apply isort on some messy imports.

pllim · 2023-11-30T00:06:37Z

@bmorris3 , I think I addressed your two concerns from today. Are you okay with the remaining changes?

now due to circular call from/to Orientation plugin. In the future, this can be moved to the plugin entirely. Make imviz.link_data call plugin API directly for consistency.

bmorris3

This looks good, thank you! One question below.

bmorris3 · 2023-12-01T18:18:42Z

jdaviz/configs/imviz/helper.py

+        from jdaviz.configs.imviz.plugins.orientation.orientation import link_type_msg_to_trait
+        plg = self.plugins["Orientation"]
+        plg._obj.linking_in_progress = True
+        plg.link_type = link_type_msg_to_trait[link_type]
+        plg._obj.wcs_use_fallback = wcs_fallback_scheme == 'pixels'
+        plg.wcs_use_affine = wcs_use_affine
+        plg._obj.linking_in_progress = False
+        plg._obj._update_link()


I'm not sure I understand this change. orientation._update_link calls orientation._link_image_data which calls link_image_data. So it looks like this is a longer and less clear way of doing a similar thing?

I could not get link_image_data to work properly on its own when switching from pixel to WCS the first time. The viewer got stuck displaying the fake layer even though visibility is set properly when I check the viewer state. I think there is some magic the plugin is doing that the API alone isn't, and it is very hard to trace with the circular callbacks and async event handling. In the end, I convinced myself that this way makes the most sense to make imviz.link_data(link_type="wcs") to work properly again as advertised in the notebook.

Can/should we deprecate imviz.link_data and schedule consolidating the logic (not necessarily here)?

I was able to reproduce the behavior you're referring to, where the WCS-only layer is visible when calling link_image_data , by reverting the highlighted lines above. I'm a bit concerned that this does not work in this PR, but it does work in the main PR – what caused the change in this PR? Investigating a bit.

Can/should we deprecate imviz.link_data

That would need PO inputs. I think Erik loved the link_data API and it is widely advertised. So to ask him to now run several lines instead of one line in the API might be a non-starter. Regardless, it can be a future ticket and does not block Brett's work.

this does not work in this PR, but it does work in the main PR

Oh, no. Did I try to fix one thing but broke another thing. If you can find out what, would be great. I went down several rabbit holes last night and then gave up. It bothers me that the plugin calls link_image_data but then link_image_data has a hook to call the plugin back. I think we should get rid of link_image_data. I almost did it here but then the diff would be horrible, so I didn't.

jdaviz/configs/imviz/helper.py

Co-authored-by: Brett M. Morris <[email protected]>

pllim mentioned this pull request Nov 15, 2023

ENH: Image rotation via WCS-only layers spacetelescope/jdaviz#2179

Merged

11 tasks

pllim commented Nov 16, 2023

View reviewed changes

jdaviz/configs/imviz/plugins/viewers.py Show resolved Hide resolved

pllim commented Nov 17, 2023

View reviewed changes

bmorris3 requested changes Nov 27, 2023

View reviewed changes

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

pllim force-pushed the pr-to-pr2179 branch from 7292c58 to 0d39a51 Compare November 28, 2023 22:52

pllim force-pushed the pr-to-pr2179 branch from 0d39a51 to 3e47e62 Compare November 28, 2023 23:01

This comment was marked as resolved.

Sign in to view

pllim added 2 commits November 28, 2023 19:04

Fix rebase weirdness maybe

92c1da7

We do not need the concept notebook anymore

26ff880

because there is GUI

pllim added 4 commits November 29, 2023 13:22

Revert removal of linked pan/zoom new code.

e07b603

Do not assume data_collection[0] is always real data.

Fix astrowidgets markers.

69295ff

Fix linking in batch mode. Apply isort on some messy imports.

clean up linked pan/zoom code

e0abdbc

Fix change log

753bc2a

This comment was marked as resolved.

Sign in to view

pllim added 2 commits November 30, 2023 21:58

Privatize link_image_data because it is breaky

58b84be

now due to circular call from/to Orientation plugin. In the future, this can be moved to the plugin entirely. Make imviz.link_data call plugin API directly for consistency.

Fix linked pan/zoom again

0db0afe

pllim marked this pull request as ready for review December 1, 2023 04:38

bmorris3 reviewed Dec 1, 2023

View reviewed changes

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

Let's hope for the best

41f33b2

Co-authored-by: Brett M. Morris <[email protected]>

bmorris3 merged commit d648eec into bmorris3:wcs-only-layers Dec 1, 2023
10 of 12 checks passed

pllim deleted the pr-to-pr2179 branch December 1, 2023 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Fix blockers #22

WIP: Fix blockers #22

pllim commented Nov 15, 2023 •

edited

Loading

pllim Nov 16, 2023 •

edited

Loading

pllim Nov 17, 2023

pllim Nov 17, 2023

codecov-commenter commented Nov 17, 2023 •

edited

Loading

This comment was marked as resolved.

pllim commented Nov 29, 2023

pllim commented Nov 30, 2023

This comment was marked as resolved.

bmorris3 left a comment

bmorris3 Dec 1, 2023

pllim Dec 1, 2023

kecnry Dec 1, 2023

bmorris3 Dec 1, 2023

pllim Dec 1, 2023

WIP: Fix blockers #22

WIP: Fix blockers #22

Conversation

pllim commented Nov 15, 2023 • edited Loading

TODO

pllim Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

pllim Nov 17, 2023

Choose a reason for hiding this comment

pllim Nov 17, 2023

Choose a reason for hiding this comment

codecov-commenter commented Nov 17, 2023 • edited Loading

Codecov Report

This comment was marked as resolved.

pllim commented Nov 29, 2023

pllim commented Nov 30, 2023

This comment was marked as resolved.

bmorris3 left a comment

Choose a reason for hiding this comment

bmorris3 Dec 1, 2023

Choose a reason for hiding this comment

pllim Dec 1, 2023

Choose a reason for hiding this comment

kecnry Dec 1, 2023

Choose a reason for hiding this comment

bmorris3 Dec 1, 2023

Choose a reason for hiding this comment

pllim Dec 1, 2023

Choose a reason for hiding this comment

pllim commented Nov 15, 2023 •

edited

Loading

pllim Nov 16, 2023 •

edited

Loading

codecov-commenter commented Nov 17, 2023 •

edited

Loading