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

Added examples for mode arg in geometric_transform #8126

Merged
merged 4 commits into from Nov 8, 2017

Conversation

@mmxmb
Copy link
Contributor

commented Nov 6, 2017

IMO, it is unclear what some modes do exactly so I included examples from ni_support.c.

Added examples for mode arg in geometric_transform
IMO, it is unclear what some modes do exactly so I included examples from [ni_support.c](https://github.com/scipy/scipy/blob/master/scipy/ndimage/src/ni_support.c).
@person142

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

Maybe better to add actual code examples to the "Examples" section?

@mmxmb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2017

@person142

Something like this? I think it's not as concise and straightforward as an analogy with a char vector that is used in ni_support.c, especially for the 'nearest' mode.

Examples
--------

>>> a = np.arange(12.).reshape((4, 3))
>>> def shift_func(output_coords):
...     return (output_coords[0] - 0.5, output_coords[1] - 0.5)
...
>>> sp.ndimage.geometric_transform(a, shift_func, mode='constant')
array([[ 0.   ,  0.   ,  0.   ],
       [ 0.   ,  1.362,  2.738],
       [ 0.   ,  4.812,  6.187],
       [ 0.   ,  8.263,  9.637]])
>>> scipy.ndimage.interpolation.geometric_transform(a, shift_func, mode='nearest')
array([[ -8.32667268e-16,   3.12500000e-01,   1.68750000e+00],
       [  1.05000000e+00,   1.36250000e+00,   2.73750000e+00],
       [  4.50000000e+00,   4.81250000e+00,   6.18750000e+00],
       [  7.95000000e+00,   8.26250000e+00,   9.63750000e+00]])
>>> scipy.ndimage.interpolation.geometric_transform(a, shift_func, mode='reflect')
array([[ 1.3625,  1.3625,  2.7375],
       [ 1.3625,  1.3625,  2.7375],
       [ 4.8125,  4.8125,  6.1875],
       [ 8.2625,  8.2625,  9.6375]])
>>> scipy.ndimage.interpolation.geometric_transform(a, shift_func, mode='wrap')
array([[ 9.6375,  8.2625,  9.6375],
       [ 2.7375,  1.3625,  2.7375],
       [ 6.1875,  4.8125,  6.1875],
       [ 9.6375,  8.2625,  9.6375]])
@person142

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

I was thinking more like this:

>>> import numpy as np
>>> from scipy.ndimage import geometric_transform
>>> def shift(coords):
...     return (coords[0] - 3,)
...
>>> geometric_transform([1, 2, 3, 4, 5], shift, mode='constant')
array([0, 0, 0, 1, 2])
>>> geometric_transform([1, 2, 3, 4, 5], shift, mode='nearest')
array([1, 1, 1, 1, 2])
>>> geometric_transform([1, 2, 3, 4, 5], shift, mode='reflect')
array([3, 2, 1, 1, 2])
>>> geometric_transform([1, 2, 3, 4, 5], shift, mode='wrap')
array([2, 3, 4, 1, 2])

You may be right though.

@mmxmb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2017

@person142

Oh, I was defining the return value for the function incorrectly. I also tried making a 1D example and couldn't make it to work.

Your example is pretty concise, so I think we should use it. You are right that examples should be in the appropriate section.

I expected the output of your last call to geometric_transform([1, 2, 3, 4, 5], shift, mode='wrap') to be array([3, 4, 5, 1, 2]), so it is helpful that this example shows some specific detail about the implementation.

@person142 person142 requested a review from jaimefrio Nov 6, 2017

@person142

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

I expected the output of your last call to geometric_transform([1, 2, 3, 4, 5], shift, mode='wrap') to be array([3, 4, 5, 1, 2])

Me too actually. I might be wrong, but I think the current behavior is a bug. Probably @jaimefrio would know better than me?

Added examples for mode arg in geometric_transform
Moved suggested example to "Examples" section.

@person142 person142 added this to the 1.1.0 milestone Nov 7, 2017

@person142 person142 removed the request for review from jaimefrio Nov 7, 2017

array([[ 0. , 0. , 0. ],
[ 0. , 1.362, 2.738],
[ 0. , 4.812, 6.187],
[ 0. , 8.263, 9.637]])
>>> b = [1, 2, 3, 4, 5]

This comment has been minimized.

Copy link
@person142

person142 Nov 7, 2017

Member

Two tiny revisions (just to make it look pretty).

  • Add a blank line between the two examples so that they don't run together:

https://4828-1460385-gh.circle-artifacts.com/0/home/ubuntu/scipy/doc/build/html-scipyorg/generated/scipy.ndimage.geometric_transform.html#scipy.ndimage.geometric_transform

  • Should probably also call the second shift function shift_func too just so that it's consistent.

This comment has been minimized.

Copy link
@mmxmb

mmxmb Nov 8, 2017

Author Contributor

Fixed.

DOC: Added examples for geometric_transform
Added examples for mode arg in geometric_transform
array([[ 0. , 0. , 0. ],
[ 0. , 1.362, 2.738],
[ 0. , 4.812, 6.187],
[ 0. , 8.263, 9.637]])
>>>

This comment has been minimized.

Copy link
@person142

person142 Nov 8, 2017

Member

Ah sorry, should have been more clear here. What I meant was break up the examples like this:

...
>>> geometric_transform(a, shift_func)
array([[ 0.   ,  0.   ,  0.   ],
           [ 0.   ,  1.362,  2.738],
           [ 0.   ,  4.812,  6.187],
           [ 0.   ,  8.263,  9.637]])

>>> b = [1, 2, 3, 4, 5]
...

Then each example will get rendered in a separate gray box. (See e.g. https://docs.scipy.org/doc/scipy/reference/generated/scipy.special.diric.html#scipy.special.diric and the source https://github.com/scipy/scipy/blob/v1.0.0/scipy/special/basic.py#L80.)

@person142 person142 merged commit 1f69954 into scipy:master Nov 8, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 87.98% (target 1%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@person142

This comment has been minimized.

Copy link
Member

commented Nov 8, 2017

CI failure is unrelated, so merging. Thanks @LawnboyMax!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.