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

Fix syn-3d example #703

Merged
merged 3 commits into from Aug 31, 2015

Conversation

Projects
None yet
3 participants
@omarocegueda
Contributor

omarocegueda commented Aug 14, 2015

We recently renamed some utility functions (those that apply affine transforms) from 'warp...' to 'transform...', but did not update the tutorial appropriately. Here we use AffineMap instead of directly calling the utility function to make the code a bit cleaner.

@arokem

This comment has been minimized.

Member

arokem commented Aug 15, 2015

I think that these changes break backwards compatibility with the 0.9 release. I think that we should transition more gently: put a deprecation warning for now, but keep the previous functionality, and make these names go away only on the 0.11 release. What do you think?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 15, 2015

It's a good procedure to keep backwards compatibility. But it may be an overkill here to worry about a single transform function introduced only in the previous release. So, my take is don't worry about backwards compatibility with this one. But definitely say in doc/api_changes.rst that this change happened.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 15, 2015

But also it doesn't cost much to have to remove this function in the 0.11 release. The problem with this specific function is that the name is misleading. It says warp although it is only about affine transformation. I wouldn't want people to get used to warp* terminology. So maybe the sooner we get rid of it the better it will be.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 15, 2015

For me this kind of change is more like a BF rather than RF. Good naming is crucial for a good API.

@arokem

This comment has been minimized.

Member

arokem commented Aug 15, 2015

OK - agreed.

This does seem to be a fix, but maybe make a note in the docs, for (the
few) people who transition from 0.9 to 0.10 and have their code start
breaking...

On Fri, Aug 14, 2015 at 6:28 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

For me this kind of like more like a BF here rather than RF. Good naming
is crucial for an API.


Reply to this email directly or view it on GitHub
#703 (comment).

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 15, 2015

Sure! :)

@arokem

This comment has been minimized.

Member

arokem commented Aug 27, 2015

Hey @omarocegueda - just a gentle reminder to put a note in the docs (here: https://github.com/nipy/dipy/blob/master/doc/api_changes.rst), so that we can merge this.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Aug 27, 2015

Thank you Ariel! I had forgotten about this! I just added the note. Actually, this also reminds me about these other utility functions from the imaffine module:
align_centers_of_mass
align_geometric_centers
align_origins
These have not been released, we are ok. @Garyfallidis suggested to rename them to transform_* instead of align_*. These functions just return the affine that must be applied to the moving image to align their geometric centers, centers of mass and origins, respectively (they are not applying the transform, just returning the matrix). Do you guys agree to rename them to transform_*?

@arokem

This comment has been minimized.

Member

arokem commented Aug 28, 2015

I think that renaming is a good idea. Could we continue to support the align_* functions for the next release and raise a deprecation warning when the old name is used (could probably be done with a deprecation decorator of some sort?)? These are not really bugs, but really just mis-namings that should be corrected.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Aug 31, 2015

No problem! =) I followed the example from aniso2iso.py.

@arokem

This comment has been minimized.

Member

arokem commented Aug 31, 2015

LGTM. What do you think, @Garyfallidis ?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 31, 2015

Looks good. Thx!

Garyfallidis added a commit that referenced this pull request Aug 31, 2015

@Garyfallidis Garyfallidis merged commit f7da857 into nipy:master Aug 31, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment