Add OpenCV as an option for rotation #6089

ayshih · 2022-04-19T20:12:41Z

Following Overhaul of affine_transform() and GenericMap.rotate() #5867, this PR adds OpenCV as an option for rotating images or maps (by specifying method='cv2'). This closes OpenCV as alternative library for map.image.affine_transform #4478. See also Changes to map.rotate: Compatibility with custom affine_transforms via method= argument #4502 and Update affine_transform with openCV support #6071.
Changed the default interpolation order for GenericMap.rotate() from 4 to 3
Make the definition of supported values for order part of the rotation-function registration

To do:

Figure out some sane way to link to the documentation for cv2.warpAffine()
~~Fix the bug in pytest-mpl for generating the HTML summary when an image comparison fails due to a mismatch in image dimensions~~ Bug is present in pytest-mpl 0.14.0, but is not present in the dev branch.

sunpy/image/transform.py

cbard · 2022-04-19T21:50:44Z

Hey @ayshih ! Thanks for digging out my old PR and incorporating it into your rotate redesign!

ayshih · 2022-04-20T03:14:28Z

Rats, my implementation isn't working correctly on non-square pixels... Stay tuned

ayshih · 2022-04-20T03:22:47Z

To be clear, I mean non-square pixels that are not aligned with the coordinate axes, that is a PCij matrix that is not a pure rotation (see #5784/#5803)

setup.cfg

ayshih · 2022-04-20T13:43:50Z

The bug with rotating by a PCij matrix that is not a pure rotation has been fixed. It's neat that parametrizing a figure test works.

The figure-test CI builds are currently failing due to a bug that is present in the latest stable release of pytest-mpl (0.14.0).

ayshih · 2022-04-20T13:50:48Z

There is a quirk in my implementation here where I allow for order=4 even though OpenCV doesn't implement biquartic, and simply have it use bicubic (i.e., order=4 and order=3 are equivalent). The reason is that I want OpenCV rotation to Just Work with the default for GenericMap.rotate(), which is order=4.

This may be an opportune time to reconsider the change in #1185 (by me, for SunPy 0.7!), and perhaps revert back to a default of order=3. I haven't yet investigated, but maybe the switch in #5867 to defaulting to SciPy mitigates the concerns that had been raised with order=3 as the default. (My comment #1074 (comment) suggests that it would.) (See also #1113)

ayshih · 2022-04-20T14:19:30Z

I went ahead and switched GenericMap.rotate() to default to order=3.

Cadair · 2022-04-20T19:14:51Z

sunpy/image/transform.py

@@ -325,8 +346,58 @@ def _rotation_skimage(image, matrix, shift, order, missing, clip):
    return rotated_image


+@add_rotation_function("cv2", allowed_orders={0, 1, 3},


Why 'cv2' and not 'opencv'?

<shrug> Extrapolating from the single point of 'skimage' instead of 'scikit-image', it seems that we are preferring the import name.

I also like "cv2" over "opencv".

I really don't like "cv2" it's completely impenetrable to people who haven't used opencv before, unlike opencv it doesn't give you sensible things when you search for it (unlike skimage which is the first result). While it might be the module name, I would argue that whoever decided that you should do import cv2 was also not of sound mind.

If I recall correctly, the reason that the package is called cv2 is because they changed the API from their original cv package. It has nothing to do with the version of OpenCV.

If we want, we could actually support both 'cv2' and 'opencv'. We could also support both 'skimage' and 'scikit-image'. Why not!

changelog/6089.feature.rst

…function

ayshih · 2022-04-21T04:31:47Z

I added an initial commit that preserves @cbard's authorship at some level.

I added (potentially objectionable) scraping of the OpenCV docs to construct the link to their docstring for cv2.warpAffine() for their latest stable version. The link can't be styled exactly the same as intersphinx links, lest it trigger an intersphinx warning due to not having an inventory for cv2.

ayshih · 2022-04-21T04:54:22Z

I'll point out that even though the figure-test CI builds are crashing (with pytest-mpl 0.14.0), that's only because the HTML summary page can't be generated when there is mismatch in image dimensions between the baseline and the new images. Once this PR is merged, its images will become the new baseline, so there won't be a persisting crashing of the figure-test CI builds.

dstansby

Looks good - I've left some comments that shouldn't block feature freeze, so if they aren't read/replied to by this afternoon I will merge this and open a new issue with my comments.

dstansby · 2022-04-22T10:13:00Z

docs/conf.py

+    # In the event of any failure (e.g., no network connectivity)
+    warpAffine_full = ""
+
+rst_epilog = f"""


Putting this at the end of every .rst file seems a bit much - I presume there's not an easy way to put this just on the pages that it's needed?

It's not particularly an issue putting it everywhere, and putting it everywhere means that you don't have to do anything to use it.

A different approach would be to programmatically add it to the specific docstring where it is needed, but that would be fiddly if we ever rename the function or want to use the shortcut in more places. If this showed up in the rendered pages, it'd of course be untenable, but it's essentially invisible to the reader.

changelog/6089.breaking.rst

docs/whatsnew/4.0.rst

sunpy/image/tests/test_transform.py

dstansby · 2022-04-22T10:16:58Z

sunpy/image/tests/test_transform.py

 @pytest.mark.parametrize('order', range(6))
 def test_endian(method, order, rot30):
+    if order not in _rotation_registry[method].allowed_orders:
+        return


Do a pytest.skip with a suitable message here instead?

Hmm, yes, that could be done, but wouldn't that just add noise to the short summary of every test run? There'd always be skips reported because these orders are simply not supported. I'm inclined to consider them passes rather than skips.

I am with Albert on this, adding more skip test results at the end of the test output is not something I support especially since we might never be able to unskip them.

sunpy/image/transform.py

Cadair

I am really struggling to get past cv2 over opencv, and the arguments to add_rotation_function should be keyword only arguments.

ayshih · 2022-04-22T15:42:11Z

As discussed on Element, the rotation method names have been changed from 'skimage' and 'cv2' to 'scikit-image' and 'opencv'. Since the method keyword was only recently introduced (#5916) and hasn't actually been released, I did not add a changelog entry for the change to the string used to select scikit-image. I wanted to modify that PR's changelog entry anyway because it explicitly stated that scikit-image was the default, but that is changed by this PR.

cbard · 2022-04-22T20:07:18Z

Thanks to everyone here for their work in getting OpenCV into Sunpy! I also want to acknowledge Raphael Attie (@WaaallEEE), whose work during the HelioHackweek2020 demonstrated the utility of OpenCV as a worthwhile alternative to skimage and scipy and led me to create the original PR #4502.

ayshih added map Affects the map submodule image Affects the image submodule No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) labels Apr 19, 2022

ayshih force-pushed the opencv branch from 03dd275 to e89bf75 Compare April 19, 2022 20:13

nabobalis reviewed Apr 19, 2022

View reviewed changes

sunpy/image/transform.py Outdated Show resolved Hide resolved

nabobalis added this to the 4.0.0 milestone Apr 19, 2022

nabobalis mentioned this pull request Apr 19, 2022

Changes to map.rotate: Compatibility with custom affine_transforms via method= argument #4502

Closed

cbard reviewed Apr 19, 2022

View reviewed changes

sunpy/image/transform.py Outdated Show resolved Hide resolved

ayshih force-pushed the opencv branch from 86117e7 to dbdd53c Compare April 20, 2022 04:17

Cadair reviewed Apr 20, 2022

View reviewed changes

setup.cfg Show resolved Hide resolved

ayshih force-pushed the opencv branch from 406e6d8 to 47a1efc Compare April 20, 2022 13:34

ayshih force-pushed the opencv branch from 47a1efc to 643a4da Compare April 20, 2022 14:16

ayshih force-pushed the opencv branch 3 times, most recently from b9e2aef to 84981c4 Compare April 20, 2022 17:51

Cadair reviewed Apr 20, 2022

View reviewed changes

ayshih marked this pull request as ready for review April 20, 2022 20:21

ayshih requested review from a team as code owners April 20, 2022 20:21

nabobalis reviewed Apr 20, 2022

View reviewed changes

changelog/6089.feature.rst Outdated Show resolved Hide resolved

cbard and others added 4 commits April 21, 2022 00:20

Partial implementation for adding OpenCV as a rotation method

3638129

Complete the implementation of OpenCV as a rotation option

b6555ae

Changed the default interpolation for GenericMap.rotate() from 4 to 3

e4d6e99

Declare the allowed interpolation orders when registering a rotation …

7158cb0

…function

Creative way to link to OpenCV docs for warpAffine

3c0c56a

ayshih force-pushed the opencv branch from 84981c4 to 3c0c56a Compare April 21, 2022 04:26

Cadair self-requested a review April 21, 2022 07:35

nabobalis removed request for a team April 21, 2022 16:25

nabobalis approved these changes Apr 21, 2022

View reviewed changes

dstansby approved these changes Apr 22, 2022

View reviewed changes

Cadair requested changes Apr 22, 2022

View reviewed changes

nabobalis approved these changes Apr 22, 2022

View reviewed changes

nabobalis requested a review from Cadair April 22, 2022 15:43

ayshih force-pushed the opencv branch from 930f37e to ec0e18c Compare April 22, 2022 15:49

Renamed the rotation methods and addressed comments

c7fd127

ayshih force-pushed the opencv branch from ec0e18c to c7fd127 Compare April 22, 2022 15:56

Cadair approved these changes Apr 22, 2022

View reviewed changes

nabobalis merged commit 299198c into sunpy:main Apr 22, 2022

nabobalis mentioned this pull request Apr 22, 2022

OpenCV as alternative library for map.image.affine_transform #4478

Closed

ayshih mentioned this pull request Apr 24, 2022

Significant improvements in rotation performance #6099

Merged

ayshih deleted the opencv branch October 10, 2022 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenCV as an option for rotation #6089

Add OpenCV as an option for rotation #6089

ayshih commented Apr 19, 2022 •

edited

Loading

cbard commented Apr 19, 2022

ayshih commented Apr 20, 2022

ayshih commented Apr 20, 2022

ayshih commented Apr 20, 2022

ayshih commented Apr 20, 2022 •

edited

Loading

ayshih commented Apr 20, 2022

Cadair Apr 20, 2022

ayshih Apr 20, 2022

nabobalis Apr 20, 2022

Cadair Apr 22, 2022

ayshih Apr 22, 2022

ayshih commented Apr 21, 2022

ayshih commented Apr 21, 2022

dstansby left a comment

dstansby Apr 22, 2022

Cadair Apr 22, 2022

ayshih Apr 22, 2022

dstansby Apr 22, 2022

ayshih Apr 22, 2022

nabobalis Apr 22, 2022

Cadair left a comment

ayshih commented Apr 22, 2022

cbard commented Apr 22, 2022

		@@ -325,8 +346,58 @@ def _rotation_skimage(image, matrix, shift, order, missing, clip):
		return rotated_image


		@add_rotation_function("cv2", allowed_orders={0, 1, 3},

Add OpenCV as an option for rotation #6089

Add OpenCV as an option for rotation #6089

Conversation

ayshih commented Apr 19, 2022 • edited Loading

cbard commented Apr 19, 2022

ayshih commented Apr 20, 2022

ayshih commented Apr 20, 2022

ayshih commented Apr 20, 2022

ayshih commented Apr 20, 2022 • edited Loading

ayshih commented Apr 20, 2022

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

ayshih commented Apr 21, 2022

ayshih commented Apr 21, 2022

dstansby left a 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

Cadair left a comment

Choose a reason for hiding this comment

ayshih commented Apr 22, 2022

cbard commented Apr 22, 2022

ayshih commented Apr 19, 2022 •

edited

Loading

ayshih commented Apr 20, 2022 •

edited

Loading