Proposed changes for PR 2179 (ENH: Image rotation via WCS-only layers) #4

pllim · 2023-05-17T04:52:02Z

codecov-commenter · 2023-05-17T05:06:42Z

Codecov Report

❗ No coverage uploaded for pull request base (wcs-only-layers@3e3d570). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@                Coverage Diff                 @@
##             wcs-only-layers       #4   +/-   ##
==================================================
  Coverage                   ?   91.74%           
==================================================
  Files                      ?      147           
  Lines                      ?    16447           
  Branches                   ?        0           
==================================================
  Hits                       ?    15089           
  Misses                     ?     1358           
  Partials                   ?        0

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

pllim · 2023-05-17T17:28:16Z

jdaviz/configs/imviz/wcs_utils.py

-    # multiplying by +/-1 can flip north/south or east/west
-    flip_axes = models.Multiply(cdelt_signs[0]) & models.Multiply(cdelt_signs[1])
+    # Multiplying by +/-1 can flip north/south or east/west.
+    e_sign = -1  # We want E-left by default.


This might be subtle enough that you might miss it scrolling through. Does this look correct? I was playing with different ways to make N-up E-left and this was what worked for me.

The way that I call rotated_gwcs in _prepare_rotated_nddata is meant to do what you want here, with an approach that sets N and E positive by default for any input WCS.

jdaviz/jdaviz/configs/imviz/wcs_utils.py

Lines 387 to 394 in 95205d4

# flip e.g. RA or Dec axes?

cdelt_signs = np.sign(wcs.wcs.cdelt)

# create a GWCS centered on ``filename``,

# and rotated by ``rotation_angle``:

new_rotated_gwcs = rotated_gwcs(

central_world_coord, rotation_angle, pixel_scales, cdelt_signs

)

Did this not work for you without the extra flip from e_sign? If so, I'm confused, because I've been checking the horsehead orientation in N-up/E-left against other vis tools and I think my demo recording in spacetelescope#2179 is correct.

In my concept notebook, without this sign flip, it rotates not the way I thought it would when the angle is 0 * u.deg. But I don't understand the subtleties of all the different WCS conventions out there. The WCS I use in the concept notebook is the same as the ones here:

https://github.com/spacetelescope/jdaviz/blob/135ae31c0acad06abe9af60d5bbbfa8eb1c29010/jdaviz/configs/imviz/tests/utils.py#L98

I guess I'm wondering how you know what you expect. The data I'm loading have a ground truth (sky truth?) that you can check, i.e. here. If you run my notebook with rot=0*u.deg, you see the same orientation as Aladin.

how you know what you expect

The problem is I don't. I naively thought simply passing in 0 * u.deg would give me N-up E-left no matter what. Isn't that what users would expect too? We don't know what WCS they will have, but they expect zero angle means certain sky orientation.

Can you please clarify a little more here? For instance, once you have degn, do you pass it in as-is to _get_rotated_nddata_from_label or you pass in -degn? I just want to understand how all these parts are going to play together.

I haven't worked with those methods yet so I don't already know. It looks like get_compass_info will return the angle from the +y direction of your reference data image towards North as degn, though it's unclear from the docstring if that's the CW or CCW angle. So your suggestion above is probably correct or needs a *(-1).

I thought I did that yesterday but it also flipped in the E-W direction for some reason. Hmm. Anyways, I guess I can revert this and chuck a "FIXME" note about this.

Though if you already know what sky orientation you want, might be easier to hardcode the WCS than to do extra math. I think a WCS with N-up E-left is pretty trivial to make by hand, thought I did get stuck on the pixel scale part which you solved, so I dunno... have to think about it maybe as a follow-up work.

To follow up, I've confirmed that this particular sign flip in your PR breaks the intended behavior. Please revert.

bmorris3

Here's my first read. I'm adding these comments so you can respond while I test your notebook and my notebook (and cycle through other tickets 🙃).

bmorris3 · 2023-05-17T17:28:59Z

jdaviz/configs/imviz/plugins/parsers.py

-            # since NDDataArray.uncertainty may be an object like
-            # StdDevUncertainty, we need to take another attr
-            # like StdDevUncertainty.array:


I had intentionally double-checked each new NDData to ensure they were meant to be WCS only by checking for the _WCS_ONLY key and that the data are all NaN. But I agree that my revision isn't strictly necessary (and yours is good), not least because I wrote the translator for the StdDevUncertainty 😉.

Would there ever be a case where user would use _WCS_ONLY with real data for some other reason? Otherwise, I don't see why we need one extra check, but maybe I am missing something here.

Would there ever be a case where user would use _WCS_ONLY with real data for some other reason?

Probably not, which is why I have no objections.

bmorris3 · 2023-05-17T17:31:39Z

jdaviz/configs/imviz/tests/test_wcs_utils.py

+        for wcs_only_data, _ in _nddata_to_glue_data(ndd, wcs_only_data_label):
+            break  # Can stop after first iteration


I found it a bit confusing the first few times I read it, but I think it's what we need and it's neat so I'm not asking for a revision. I did want to ask though – is there a clearer (if longer) alternative implementation for this?

Well, I think I brought this upon myself. I thought implementing _*_to_glue_data as iterator was clever because you process each loop one by one and don't have to read everything into memory all at once before processing the data. But as a result, when there is only one loop, you have to do weird things to it.

The cleaner way is to copy paste only that few lines from inside _nddata_to_glue_data to wherever this is called in production; for instance, you can throw away the unit handling stuff. But that means you are duplicating the parser code. But if that makes things easier to maintain going forward, so be it.

I am still hesitant to touch the actual parsers.py because that is meant for loading user data.

jdaviz/configs/imviz/wcs_utils.py

bmorris3 · 2023-05-17T17:35:16Z

jdaviz/configs/imviz/wcs_utils.py

-        data=np.nan * np.ones(refdata_shape),
+    # create a fake NDData with the rotated GWCS:
+    ndd = NDData(
+        data=np.empty(refdata_shape, dtype=np.int8),


I used NaNs here to be sure that if a user finds a way to extract those data, they aren't ever misinterpreted as something meaningful. Is there a motivation to use empty instead?

Empty is faster because numpy skips populating the array. Is there really any risk of someone seeing a 2 x 2 array with values like this think it is meaningful?

array([[1.5e-323, 0.0e+000], [4.9e-324, 9.9e-324]])

Though you could also argue that initializing a 2 x 2 array is not a performance concern. So if you really want them to be NaN, I can revert this change, but personally I don't think the content matters here.

I'd advocate for revert. I agree that if you see this array in a notebook, it's clear something funny is going on. My concern is that if one of these layers were naively plotted inside jdaviz or outside in mpl for example, it would look like a uniform near-zero image. Since all values are about the same, you wouldn't be able to see the separate pixels and notice that there are only 2x2 pixels. So the nans are just meant to signal "this isn't the data that you want" by one more means.

Actually, you cannot see "nan" in Imviz either (so it is basically setting values to zeros as far as visibility is concerned). It is only obvious when you mouse over one of the 4 pixels that the value is "nan". In fact, any homogenous array won't show (even when I use np.ones). So if the goal is for user to see immediately they are looking at a 2 x 2 array that cannot be real, I would suggest np.arange(4).reshape((2, 2)).

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

jdaviz/configs/imviz/plugins/parsers.py

bmorris3 · 2023-05-18T13:44:08Z

I think there are a couple of things I am supposed to change here but I'll wait till you have done testing it. Thanks!

All done, feel free to update whenever you're ready 👍🏻

pllim · 2023-05-19T00:42:00Z

@bmorris3 , I think I made the changes you requested.

I didn't see any mention of not using imviz.default_viewer, so I guess that is okay now?

pllim · 2023-05-19T00:43:17Z

Even though the concept notebook isn't exactly what you envisioned, I still find it useful for debugging. But feel free to update it. I just want something other devs can play with until this concept can become a real plugin.

Thanks for your patience!

* corrections to the pr to the pr to the pr * using app-level attr * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> --------- Co-authored-by: P. L. Lim <[email protected]>

into viewer but not visible. Fix concept notebook

pllim · 2023-05-19T22:16:36Z

OK your changes incorporated and I think things work again.

Since I won't be available much this week, I think we have agreed that your ticket is done when this is merged? Maybe confirm with @kecnry

bmorris3

Great, thanks!

bmorris3 · 2023-05-19T16:39:03Z

jdaviz/configs/imviz/wcs_utils.py

+    # create a fake NDData (we use arange so data boundaries show up in Imviz
+    # if it ever is accidentally exposed) with the rotated GWCS:
+    ndd = NDData(
+        data=np.arange(sum(refdata_shape), dtype=np.int8).reshape(refdata_shape),


Suggested change

data=np.arange(sum(refdata_shape), dtype=np.int8).reshape(refdata_shape),

data=np.arange(np.prod(refdata_shape), dtype=np.int8).reshape(refdata_shape),

bmorris3

Great, thanks!

#4) * Lim edits * Improve verbiage in docstring Co-authored-by: Brett M. Morris <[email protected]> * Address review comments from bmorris3 * Add multi WCS-only test back * Update concept notebook * corrections to the pr to the pr to the pr (#10) * corrections to the pr to the pr to the pr * using app-level attr * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> --------- Co-authored-by: P. L. Lim <[email protected]> * Follow up to PR to my PR to your PR * WCS-only must be loaded into viewer but not visible. Fix concept notebook --------- Co-authored-by: Brett M. Morris <[email protected]>

Lim edits

e2d09be

pllim requested a review from bmorris3 as a code owner May 17, 2023 04:52

github-actions bot added documentation Improvements or additions to documentation imviz labels May 17, 2023

pllim mentioned this pull request May 17, 2023

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

Merged

11 tasks

pllim commented May 17, 2023

View reviewed changes

bmorris3 reviewed May 17, 2023

View reviewed changes

Improve verbiage in docstring

9ca2eda

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

This comment was marked as resolved.

Sign in to view

bmorris3 reviewed May 18, 2023

View reviewed changes

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

pllim added 2 commits May 18, 2023 20:15

Address review comments from bmorris3

6800fff

Add multi WCS-only test back

e2d384b

Update concept notebook

920a526

bmorris3 mentioned this pull request May 19, 2023

corrections to the pr to the pr to the pr pllim/jdaviz#10

Merged

bmorris3 and others added 2 commits May 19, 2023 16:54

Follow up to PR to my PR to your PR

6b2eb38

This comment was marked as resolved.

Sign in to view

WCS-only must be loaded

bf8b3bf

into viewer but not visible. Fix concept notebook

bmorris3 approved these changes May 22, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

bmorris3 approved these changes May 22, 2023

View reviewed changes

bmorris3 merged commit 75beece into bmorris3:wcs-only-layers May 22, 2023

pllim deleted the pr-to-pr2179 branch May 22, 2023 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposed changes for PR 2179 (ENH: Image rotation via WCS-only layers) #4

Proposed changes for PR 2179 (ENH: Image rotation via WCS-only layers) #4

pllim commented May 17, 2023

codecov-commenter commented May 17, 2023 •

edited

Loading

pllim May 17, 2023

bmorris3 May 17, 2023

pllim May 17, 2023

bmorris3 May 17, 2023

pllim May 17, 2023

pllim May 17, 2023

bmorris3 May 17, 2023

pllim May 17, 2023

pllim May 17, 2023

bmorris3 May 18, 2023

bmorris3 left a comment

bmorris3 May 17, 2023 •

edited

Loading

pllim May 17, 2023

bmorris3 May 17, 2023

bmorris3 May 17, 2023

pllim May 17, 2023

bmorris3 May 17, 2023

pllim May 17, 2023

bmorris3 May 17, 2023

pllim May 17, 2023

This comment was marked as resolved.

bmorris3 commented May 18, 2023

pllim commented May 19, 2023

pllim commented May 19, 2023

This comment was marked as resolved.

pllim commented May 19, 2023

bmorris3 left a comment

bmorris3 May 19, 2023

This comment was marked as duplicate.

This comment was marked as duplicate.

bmorris3 left a comment

	# flip e.g. RA or Dec axes?
	cdelt_signs = np.sign(wcs.wcs.cdelt)

	# create a GWCS centered on ``filename``,
	# and rotated by ``rotation_angle``:
	new_rotated_gwcs = rotated_gwcs(
	central_world_coord, rotation_angle, pixel_scales, cdelt_signs
	)

		for wcs_only_data, _ in _nddata_to_glue_data(ndd, wcs_only_data_label):
		break # Can stop after first iteration

	data=np.arange(sum(refdata_shape), dtype=np.int8).reshape(refdata_shape),
	data=np.arange(np.prod(refdata_shape), dtype=np.int8).reshape(refdata_shape),

Proposed changes for PR 2179 (ENH: Image rotation via WCS-only layers) #4

Proposed changes for PR 2179 (ENH: Image rotation via WCS-only layers) #4

Conversation

pllim commented May 17, 2023

codecov-commenter commented May 17, 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

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

bmorris3 left a comment

Choose a reason for hiding this comment

bmorris3 May 17, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

bmorris3 commented May 18, 2023

pllim commented May 19, 2023

pllim commented May 19, 2023

This comment was marked as resolved.

pllim commented May 19, 2023

bmorris3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

This comment was marked as duplicate.

bmorris3 left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 17, 2023 •

edited

Loading

bmorris3 May 17, 2023 •

edited

Loading