WIP: Overhaul transform module

[kne42]

Refactorizations & Deprecations

• refactor transform module to transform_xy
• create new transform_rc module
• create wrapper transform module around transform_xy with automatic deprecation messages

Changes (implemented in transform_rc)

• switch to numpy-style coordinates (row, column)
• add nD support
• improve transformation machinery using scipy's LowLevelCallables

Other

• Implementation notes/benchmarks may be found in this notebook.
• Core LowLevelCallable notes may be found in this notebook.

[pep8speaks]

Hello @kne42! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file skimage/transform_xy/_warps.py:

Line 221:80: E501 line too long (86 > 79 characters)
Line 354:80: E501 line too long (80 > 79 characters)
Line 391:48: W504 line break after binary operator
Line 392:49: W504 line break after binary operator
Line 414:49: W504 line break after binary operator

• In the file skimage/transform_xy/hough_transform.py:

Line 105:80: E501 line too long (80 > 79 characters)
Line 172:24: E226 missing whitespace around arithmetic operator

• In the file skimage/transform_xy/setup.py:

Line 27:1: E305 expected 2 blank lines after class or function definition, found 1

• In the file skimage/transform_xy/tests/test_geometric.py:

Line 6:80: E501 line too long (80 > 79 characters)
Line 7:80: E501 line too long (80 > 79 characters)

• In the file skimage/transform_xy/tests/test_warps.py:

Line 86:34: E241 multiple spaces after ','
Line 87:22: E241 multiple spaces after ','
Line 87:39: E241 multiple spaces after ','

Comment last updated at 2019-04-06 00:49:31 UTC

[kne42]

cc @jni

[jni]

@kne42 Since this is going to touch so many files, it's important for the commit messages to be extremely thorough, and for the commits to be nicely separated. Specifically, I suggest making one commit for moving transform/ to transform_xy/, another for creating the alias for transform -> transform_xy, together with a deprecation message, and another for creating transform_rc.

Also, for the initial transform_rc "port", you could simply use the existing warps functions, but transpose the coordinates. This would let us actually merge this and then add nD capabilities little by little.

What do you think?

[kne42]

@jni

Since this is going to touch so many files, it's important for the commit messages to be extremely thorough, and for the commits to be nicely separated. Specifically, I suggest making one commit for moving transform/ to transform_xy/, another for creating the alias for transform -> transform_xy, together with a deprecation message, and another for creating transform_rc.

While I do agree on making the commit messages thorough, I typically prefer to group commits together with message prefixes and then squash them prior to a merge. However, I suppose that this, being the massive change that it is, warrants individual commit messages for each task at hand.

Also, for the initial transform_rc "port", you could simply use the existing warps functions, but transpose the coordinates. This would let us actually merge this and then add nD capabilities little by little.

Oooh yes I like this idea! Cleans up PR history too :P

[kne42]

Oopsies, that was an accident!

Anyways, @jni lmk if you spot me missing anything! This is a pretty large change so I'm not quite sure if I covered everything necessary/went too far.

Also, for the functions that are already nD/take in full-on images instead of just coordinates, do you think that we should put them in skimage.transform and just import them into skimage.transform_xy and skimage.transform_rc?

[jni]

@kne42 Unless I missed it, this doesn't yet handle the transform deprecation or the creation of transform_rc?

And, to answer your question, yes, I think I was too optimistic in recommending the wholesale move of transform->transform_xy. I think only the parts depending on _warp need to be moved...

[jni]

I'm kinda thinking that the examples should be updated to use transform_rc...

[jni]

Despite my initial suggestion, I think I'd like to apply a bit of smarts to this migration... Specifically, transforms that don't rely on coordinate conventions, such as this one, should not be deprecated/migrated.

In other words, to answer your question: yes I think we should only migrate the transforms that rely on x/y coordinates. I think that's a large proportion, but maybe I'm wrong...

[jni]

I'm concerned about the documentation. Generally, when we deprecate, we want to point to the new way of doing things. The only reason to keep the old way hanging around is for backwards compatibility. But anyone new who checks the docs should be pointed to the One True Way of doing things, and that's the new way.

[lagru]

Hey everyone, what is the status of this PR? Is this something you or we as a team want to pick at some point? I'm asking because I'm preparing a proposal for an Outreachy project that would focus on our transform module.

[jni]

@lagru I would say that this should at this point get subsumed into the skimage2 plans, so we can close this.

[lagru]

Fine by me. I added it to https://hackmd.io/@mkcor/HyYOmLnaO so that it's not forgotten.