Reproject plugin for Imviz

[pllim]

Description

This pull request is to implement a plugin to call reproject in Imviz, as requested by user.

Pro:

• Can run reproject from GUI (even though reproject is a completely separate package). See https://reproject.readthedocs.io/

Con:

• This plugin does not make sense for the following workflow: The data generated by reproject will align back with the original (reference) data when you link by WCS, defeating the purpose of reprojection.
• Not performant: It took almost 30 seconds on my machine to rotate a single 2k by 4k image (the first one in ImvizDitheredExample.ipynb)
• See "Blocker(s)" below.

Blocker(s):

• Does not work on GWCS until this issue is addressed: find_optimal_celestial_wcs does not accept GWCS astropy/reproject#328

TODO

• What to do with original (unrotated) image?
• Out of range float values are not JSON compliant
• Let user customize new label; have default label not be so ugly.
• Add tests.
• Make sure change log in next release section.
• Is it okay to not expose things as public API yet?
• Do we have to rename this plugin?
• Any way to make sure Compass zoom box is always correct? Seems to only happen when you reproject the smaller image and then use the reprojected smaller image as reference, and then view a larger image. We'll just open a follow-up ticket for this one to deal with later.

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

[camipacifici]

Got this. Looks like the meta data keyword is called "wcsinfo.wcsaxes".

[camipacifici]

Oh, apologies. I did not see the blocker. I will try with a non-gwcs image.

[camipacifici]

There is much more to discuss about this, but just a picky detail, the image is visible but according to the data menu it is not loaded in the viewer.

[pllim]

it is not loaded in the viewer

It should eventually autoload if you wait for it to finish. But if you do something else while it is not done, the callback might be cancelled or something, not sure (it did auto display for me). Like I said, there is a serious performance issue because reprojection is not meant to be some instant on-the-fly visualization; it is very calculation intensive (albeit an one-time overhead but after that it eats the memory).

[camipacifici]

I played a bit with this and here are some thoughts in random order:

• I could get the plugin to work on jwst images using the approximated wcs in the header. I consider the gwcs problem external.
• The reproject call is a little slower within jdaviz than outside of jdaviz, but not dramatically slower. Performance should be investigated with multiple images or multiple calls to reproject.
• If I do the reproject outside of jdaviz, I am left with an image and a new wcs, but no combined product. I can load the image into jdaviz, but I will miss the wcs. If I want both, I need one more step to write out a file with the image and its wcs before loading into jdaviz.
• If I load the original image into jdaviz and run reproject within jdaviz, I have the combined product (image+wcs) automatically (very convenient).
• If I reproject in jdaviz, then load another image, and align by wcs everything goes back to the original orientation (very invonvenient).
• If I reproject in jdaviz, then unload the original image before loading a new image, the align by wcs works like a charm and all subsequent images are north up (or aligned as I wanted). So the overhead of reproject happens only once per workflow. If the original image could be unloaded automatically when the reproject happens, I think there would be a big gain.
• I do not know if other problems will be triggered by unloading the original image.
• If I understood correctly, there is no need for the actual reproject package because the same functionality can be reproduced with just astropy.

@orifox @astrofrog thoughts?

[pllim]

I consider the gwcs problem external

Sure, but Imviz loads the GWCS when it is available. To ask Imviz to use the FITS WCS version is not possible right now. To make that possible is a separate ticket.

[pllim]

The reproject call is a little slower within jdaviz than outside of jdaviz

How did you profile this? We have to make sure we're not comparing apples to oranges here. Overhead of GUI/linking is unavoidable in Jdaviz but that is something using reproject directly does not have to worry about.

[pllim]

If I want both, I need one more step to write out a file with the image and its wcs before loading into jdaviz

You can also avoid writing a file by creating NDData object with all the info within and then loading that into Jdaviz. But yes, it is one extra step (you can also see that done in the plugin code here).

[pllim]

If the original image could be unloaded automatically when the reproject happens

This is technically possible but it opens up a can of worms for complicated workflows.

For example, you load the original image, have markers defined by pixel against the original image, then you reproject, now if you unload the original image, what do you want to happen to those markers?

Also, at this rate, why even do reproject inside Jdaviz? I still don't see the point when you can do this all outside of Jdaviz and then load the result after.

[pllim]

there is no need for the actual reproject package because the same functionality can be reproduced with just astropy

Not sure what you mean here, can you please elaborate, @camipacifici ?

[camipacifici]

I consider the gwcs problem external

Sure, but Imviz loads the GWCS when it is available. To ask Imviz to use the FITS WCS version is not possible right now. To make that possible is a separate ticket.

I was just thinking that this problem can be fixed on the reproject side so that the package does not crush with gwcs.

The reproject call is a little slower within jdaviz than outside of jdaviz

How did you profile this? We have to make sure we're not comparing apples to oranges here. Overhead of GUI/linking is unavoidable in Jdaviz but that is something using reproject directly does not have to worry about.

No profiling, just observation. Nothing major anyway.

If the original image could be unloaded automatically when the reproject happens

This is technically possible but it opens up a can of worms for complicated workflows.

For example, you load the original image, have markers defined by pixel against the original image, then you reproject, now if you unload the original image, what do you want to happen to those markers?

Also, at this rate, why even do reproject inside Jdaviz? I still don't see the point when you can do this all outside of Jdaviz and then load the result after.

Good point, I did not think about that. More use cases to consider here.
And the need inside the tool is for our fellow users who work in the stand alone app.

there is no need for the actual reproject package because the same functionality can be reproduced with just astropy

Not sure what you mean here, can you please elaborate, @camipacifici ?

Sorry, this is what I thought I understood from Tom's example reprojection. I thought he did not call the reproject package, but just the astropy routines that are used within the reproject package (the pixel-to-pixel method?).

[pllim]

Tom's example reprojection

Did this discussion happen while I was out? Can you please link to his example? Thanks!

[pllim]

users who work in the stand alone app

If we can revive that front-end canvas rotation, these users will not need the reproject plugin at all. If they insist on using reproject anyway, they will have to fire off a Python session and learn some Python. Using Python while pretending to not use Python can only go so far.

[camipacifici]

Tom's example reprojection

Did this discussion happen while I was out? Can you please link to his example? Thanks!

https://gist.github.com/astrofrog/720a8ce97700915d9dc965617ebdf874

[pllim]

The Gist looks vaguely familiar, I think it is tied to the following effort but I am not sure of the status. I think @astrofrog abandoned it due to performance issues or something?

• Fix bugs related to generic BaseCartesianData subclasses glue-viz/glue#2344
• Fix updating of image viewer when data is modified glue-viz/glue-jupyter#339
• Ensure ImViz works well with arbitrary BaseCartesianData instances #1904

[astrofrog]

@pllim - just FYI reproject v0.10.0 is out with the fixes to the WCS helper to work with GWCS. Let me know if you run into any issues here with this!

The gist linked above has the latest approach I worked on and that one was not abandoned due to performance but put on hold while the approach here was explored to see if it would be sufficient, and it sounds from the last SME tag-up like we might also be able to revive the image rotation in the front-end canvas which would be ideal, in which case the on-the-fly reprojection can be abandoned.

It is technically possible to use astropy without reproject to do the reprojection (coding it up by hand with pixel_to_pixel) but if reproject is going to be used anyway for the WCS helper then might as well use reproject for the reprojection too, and it could open the door for users to potentially select different reprojection modes (interp, exact, etc).

[codecov]

Codecov Report

Base: 92.04% // Head: 92.06% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (575227e) compared to base (892ff36).
Patch coverage: 91.74% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1949      +/-   ##
==========================================
+ Coverage   92.04%   92.06%   +0.02%     
==========================================
  Files         140      143       +3     
  Lines       15206    15313     +107     
==========================================
+ Hits        13996    14098     +102     
- Misses       1210     1215       +5     

Impacted Files	Coverage Δ
...daviz/configs/imviz/plugins/reproject/reproject.py	90.14% <90.14%> (ø)
jdaviz/configs/imviz/tests/test_reproject.py	94.11% <94.11%> (ø)
jdaviz/configs/imviz/plugins/__init__.py	100.00% <100.00%> (ø)
jdaviz/configs/imviz/plugins/reproject/__init__.py	100.00% <100.00%> (ø)
jdaviz/core/template_mixin.py	93.06% <100.00%> (+0.21%)	⬆️
jdaviz/configs/imviz/plugins/viewers.py	91.77% <0.00%> (+0.63%)	⬆️
jdaviz/core/freezable_state.py	95.83% <0.00%> (+1.38%)	⬆️

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.

[kecnry]

I wonder if wrapping in the progress spinner is the difference causing the overflow... 🤔

EDIT: it was the culprit, fixing now

[kecnry]

Yes, just pressing END triggered it.

Ok, it might be unsyncing on any keypress. Please file a follow-up issue/ticket for that and I think its safe to ignore for the purpose of this PR.

[pllim]

Okay, I think I addressed all the comments thus far, except for the plugin name that is being debated still.

Also, when I reproject the larger image in the Imviz example notebook, the image shape is (4636, 4648) and the reprojected shape is (6291, 6286). It look 42 seconds and I could hear my CPU churning away. cc @larrybradley

[kecnry]

Is the compass a blocker or a follow-up bug (thanks for reminding me with the commit to add the note to the docs)?

Besides that and the pending naming/placement decision, the code looks good to me!

[pllim]

Is the compass a blocker

Not really. It doesn't always happen. If you reproject the first (larger) image, the second image actually shows up fine on Compass, but not the other way around. I suspect this is a general Compass limitation when two images have vastly different dimensions. I can open up a follow-up ticket after this PR is merged.

[pllim]

Cami said I should move this back to draft while Ori & co ponder.

[pllim]

Superseded by #2179