ENH: Image rotation via WCS-only layers

[bmorris3]

• address Footprints plugin #2341 (comment)
• consider helper and viewer-level access to data_labels properties #2626 (comment)

Description

This PR is the first in a series that will allow users to rotate images in Imviz by synthesizing and selecting reference data with the desired sky orientation, encoded in WCS.

If the user loads an NDDataArray with all-NaN data and valid WCS, the Imviz parser will load these data as a "WCS-only" layer. These layers are never visible, even when added to the viewer. I track these layers a few ways, depending on the context where the WCS-only layer needs to be identified:

• at the Data component level, the data are all NaNs
• at the Data level, I add a meta entry WCS-ONLY=True
• at the viewer-item level, I add a wcs_only_layers list with data labels

These NDDataArray objects can be created by calling jdaviz.configs.imviz.wcs_utils.get_rotated_nddata_from_fits, which loads the WCS from a FITS header, generates a synthetic GWCS object with arbitrary rotation, and returns, an NDData with all-NaN data and the rotated WCS that can be loaded with imviz.load_data as usual.

The user can load the result of get_rotated_nddata_from_fits, and then load the real image to see it rotated immediately. Alternatively, the user can load their real data as usual:

imviz = Imviz()
imviz.load_data(filename)
imviz.show()

and then load the synthetic NDDataArray afterwards and select it as the reference data with the following:

# load one NDDataArray containing a rotated WCS:
rotation_angle = 45 * u.deg
ndd_rotated = get_rotated_nddata_from_fits(filename, rotation_angle)
refdata_label = f"rot: {rotation_angle:.2f}"
imviz.load_data(ndd_rotated, data_label=refdata_label)

# link by WCS:
imviz.link_data(link_type='wcs')

# set reference data to rotated NDDataArray:
imviz.app._change_reference_data(refdata_label)

The snippet above can be adapted to a method/button in the current rotation plugin, or new rotation plugin, etc. You can also add more than one rotated NDDataArray, and change orientations by calling imviz.app._change_reference_data(data_label).

By default, the synthetic NDDataArray has shape (2, 2), so it should have very minimal memory implications, though as @pllim points out, performance will take a hit because performance scales poorly with the number of links. (That side-effect is an unavoidable with this approach.)

Demo

The notebook used in this demo is available here.

glue_rotation_demo.mov

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

[codecov]

Codecov Report

Attention: 182 lines in your changes are missing coverage. Please review.

Comparison is base (20b9de5) 91.52% compared to head (414b632) 90.80%.

Files	Patch %	Lines
...z/configs/imviz/plugins/orientation/orientation.py	79.68%	51 Missing ⚠️
jdaviz/configs/imviz/wcs_utils.py	60.00%	42 Missing ⚠️
jdaviz/configs/default/plugins/markers/markers.py	20.40%	39 Missing ⚠️
jdaviz/app.py	88.77%	11 Missing ⚠️
jdaviz/core/user_api.py	55.55%	8 Missing ⚠️
jdaviz/configs/imviz/helper.py	93.33%	7 Missing ⚠️
jdaviz/core/astrowidgets_api.py	85.36%	6 Missing ⚠️
...viz/configs/default/plugins/subset_plugin/utils.py	95.45%	3 Missing ⚠️
jdaviz/configs/imviz/plugins/viewers.py	94.00%	3 Missing ⚠️
jdaviz/configs/imviz/tests/test_linking.py	91.89%	3 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2179      +/-   ##
==========================================
- Coverage   91.52%   90.80%   -0.73%     
==========================================
  Files         161      162       +1     
  Lines       20196    20962     +766     
==========================================
+ Hits        18485    19035     +550     
- Misses       1711     1927     +216     

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

[duytnguyendtn]

Example notebook provided works as expected and the code changes appear appropriate given the demonstration Brett gave this week. Good work! I know the rotation saga has been a long one

[pllim]

I'm interested in reviewing this but I can't get to it until well into next sprint, so I guess if another dev is happy, that's fine, but please remember to test more complicated cases like linking FITS WCS, GWCS, and no WCS all together; and have multiple viewers; adding markers, and so on.

[pllim]

Eventually, this PR also needs user doc update that goes with the feature.

Still a lot of codecov complains, but I think I heard there is a plan to add tests?

Note to self; need to play around with the following scenarios:

1. FITS WCS + GWCS + No WCS -- What if I change the reference data to "No WCS"? What if "No WCS" is actually loaded first?
2. Load a file with multiple SCI extensions using the * wildcard.
3. Load a file with multiple SCI extensions but only the second SCI extension, and then load something else (not sure what yet).
4. The HST/ACS dithered exposures.
5. One of the more complicated combo(s) above with multiple viewers. What happens when I have different images loaded on each of the viewers, and then rotate them separately? What if I try to rotate one without WCS?
6. How do Subsets behave for these cases? Do aperture photometry numbers make sense (have to avoid Ensure aperture is correct when images linked by WCS #2139 for now).
7. What about Line Profile plugin? Compass? Other plugins?

[pllim]

Without providing extension, this is going to be wrong for MEF with multiple SCI extensions. What if I want ('SCI', 2)?

[pllim]

1000 seems too magical a number. How was this tuned exactly and when should we update this number in the future?

[pllim]

I have a hard time finding the GUI component for this feature. I thought it would appear around the circled area below if I remember the demo correctly but I don't see it. The lack of documentation does not help. This is using Case 7 from concepts/imviz_compass_mpl.ipynb.

[pllim]

@kecnry , should removing the Canvas Rotation plugin be part of this PR too? This is supposed to replace that plugin, right? Won't it be too confusing to have too many ways to rotate the data?

[kecnry]

should removing the Canvas Rotation plugin be part of this PR too?

This does not actually support a full workflow for rotation (yet). For now the plan will be to disable canvas rotation for non-chrome browsers and we might consider removing it eventually if there isn't justification for both to exist side-by-side.

[pllim]

Ignoring the demo notebook that is not submitted as part of this PR, I feel like the patch is incomplete as all I can do is imviz.app._change_reference_data(...) but no way to actually ask Imviz to rotate anything (having to generate the actual WCS myself and pass it in isn't really what is being requested). Therefore, I am unable to effectively review the actual feature (image rotation). I would like to see more being added to this PR to the point we could have a MVP of this new way to rotate image (even just API without the GUI) to make sure what you are building here is actually what we need to achieve the goal.

So I am looking for these additional items:

1. Implementation of the stuff you show in demo but as part of the app. No user is going to generate the rotated WCS separately.
2. Provide some sort of API (private API is ok) that takes desired rotation angle (or some equivalent input) to allow the app to do the rotation stuff in the backend. We would need to be able to then expose this API via plugin (as future PR if you want).
3. Add docs even if it is meant for another dev, and not end users.
4. Add tests.

[pllim]

Since Brett said he has to do Roman stuff, I took the liberty to rebase this so if I have to add more stuff to it, it won't be painful.

[pllim]

There is a conflict now but please don't rebase until we resolve bmorris3#4 .

One downside of this kind of rotation compared to CSS rotation is that we cannot use this to rotate data without WCS, but does that matter to users?

[pllim]

2023-01-18: Left comments from #2179 (review) onwards.

Also, I see flickers in this use case, anyone else? Use the ImvizDitheredExample, have 2 viewers, A/B on 1st viewer, B on 2nd viewer, linked by WCS. When I use linked pan/zoom or linked box-zoom on first viewer, I see flicker on second viewer as it tries to readjust. 🐱

Disclaimer: I only ran the ImvizDitheredExample this time and didn't check all the other viz. I also didn't look at the code super carefully (I mean, who can, it is +3,043 −843 now).

[javerbukh]

Spent some time yesterday and today running this branch on my macbook and it works well! I can see some minor points that we may want to improve but IMO that can happen after a merge. I'll spend some more time looking through the code but I am able to go through a simple workflow without bugs or display errors, so I think this is in a good spot!

[pllim]

@javerbukh , do you want to give the first ✔️ then?

[pllim]

Good enough. I will open up another follow up issue about performance. 🐱

[pllim]

Thank you for all your hard work! 🥃

[camipacifici]

Woooooohoooooo!!!!!