Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add affine transforms #91

Merged
merged 14 commits into from
Jan 12, 2021
Merged

Add affine transforms #91

merged 14 commits into from
Jan 12, 2021

Conversation

haesleinhuepf
Copy link
Member

Hey Talley @tlambert03 and Juan @jni ,

this brings affine transforms to pyclesperanto + some convenience functions for scaling, rotation, translation and rigid transforms.

It doesn't support shearing for now (see #90 ) but I'm happy to add that later.

I would like to ask you to take a look at the API. It's more or less a re-implementation of the imglib2 / clij2 API. But if it makes sense, I'm happy to add additional optional parameters or functions to make it compatible to stuff in snake county. ;-)

Some interoperability is already available for scikit-image: The cle.affine_transform() function takes an skimage.transform.AffineTransform as transform parameter and converts it internally.

You can take a look at API and its usage in this notebook.

Thanks a lot in advance!

@tlambert03
Copy link
Contributor

tlambert03 commented Jan 10, 2021

This is super exciting @haesleinhuepf. Very promising, and I suspect affine transforms will be one of the most widely used parts of this library.

I would like to ask you to take a look at the API. It's more or less a re-implementation of the imglib2 / clij2 API. But if it makes sense, I'm happy to add additional optional parameters or functions to make it compatible to stuff in snake county. ;-)

As mentioned in #90 (comment), I personally would love to see the API here match the existing python ecosystem APIs of scipy.ndimage and cupyx.scipy.ndimage... (which have made attempts to match each other). I suspect reimplementing the imglib2/clij2 API here makes it easier to generate scripts that go between ImageJ and Python... but in the interest of building bridges, I feel like we need to "speak snake in snake country", "coffee in coffee country" and let the bridge be the compatibility layer in between (rather than asking one of ecosystems to get on board using the language of the other ecosystem). There are a lot of users of the scipy and cupy APIs, and a cl-based drop-in replacement would be a really amazing contribution.

I know you've had the discussion about explicitly naming axes in parameters elsewhere, but I definitely agree strongly with @jni in #49 (comment) that parameter names should not have any kind of implicit order. so I don't love angle_around_z_in_degrees, but I do like how scipy and cupy do rotate(input, angle: float, axes: Tuple[int, int]) (as mentioned in #90 (comment))

I know all this takes more work, and I'm happy to start getting more involved here (after tying up some other things), but that would be my vote.

@jni
Copy link
Contributor

jni commented Jan 11, 2021

What @tlambert03 said. 😄

Note also that in scikit-image/scikit-image#3544, I implemented that np.array(Transform) gives back the matrix, and I'm working right now on pulling that change out of that PR so that it can get merged independently. So that might "lighten the load" of what you have to do to make things compatible. 😊

@haesleinhuepf
Copy link
Member Author

Alright, in order to do this, I need to wait for another PR to be merged first ( #92 ). Maybe I merge them all together.

Nevertheless, actually basic compatibility can be achieved quite easily, even though API fine tuning might be needed in the future. See #93

@haesleinhuepf haesleinhuepf merged commit a1a864c into master Jan 12, 2021
@haesleinhuepf haesleinhuepf deleted the affine_transforms2 branch January 12, 2021 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants