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

Tests for reslicing #508

Merged
merged 9 commits into from Dec 24, 2014

Conversation

Projects
None yet
2 participants
@Garyfallidis
Member

Garyfallidis commented Dec 18, 2014

Increasing coverage and adding tests for reslicing as promised!

@@ -0,0 +1,2 @@
from dipy.align.aniso2iso import resample as reslice

This comment has been minimized.

@arokem

arokem Dec 18, 2014

Member

This has the potential to create circular dependencies

This comment has been minimized.

@arokem

arokem Dec 18, 2014

Member

Oh sorry - I see now that they are at the same level. This is just a renaming issue? Why are you doing this, actually?

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 18, 2014

Member

I have been thinking for long time that the function name (resample) and file name (aniso2iso.py) were not good names. Not descriptive enough. What this function is doing is actually reslicing and this PR is correcting that. But at the same time some people maybe prefer aniso2iso. So, in that way I keep both. I am happy for suggestions.

This comment has been minimized.

@arokem

arokem Dec 18, 2014

Member

As in other cases (where peaks_for_model comes from ), this adds (unnecessary?) complexity, and complexity adds a maintenance burden. You might remember what this means a year from now, because you wrote this, but a new developers will look at this and puzzle. If you think the original name is no good (I tend to agree with you), we can change it, and do it in the way you are proposing here, but that also means making a plan to slowly deprecate the other name. So, adding at the very least a note in the documentation warning others of this would be appropriate.

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 18, 2014

Member

Sure, agreed. So what do you think I should do here? I see two good options:

  1. copy and rename the functions in dipy.align.reslice and add deprecation warning in dipy.align.aniso2iso
  2. leave the old name as it is.
    Happy with everything you decide.

This comment has been minimized.

@arokem

arokem Dec 18, 2014

Member

I like #1, but maybe wait a day or two and let others chime in.

On Thu, Dec 18, 2014 at 2:50 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

In dipy/align/reslice.py
#508 (diff):

@@ -0,0 +1,2 @@
+from dipy.align.aniso2iso import resample as reslice

Sure, agreed. So what do you think I should do here? I see two good
options:

  1. copy and rename the functions in dipy.align.reslice and add deprecation
    warning in dipy.align.aniso2iso
  2. leave the old name as it is.
    Happy with everything you decide.


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/508/files#r22077854.

@arokem

This comment has been minimized.

arokem commented on dipy/align/aniso2iso.py in 532fc9b Dec 23, 2014

typo: 'aling' => 'align'

@arokem

This comment has been minimized.

Member

arokem commented Dec 23, 2014

Looks good. Would you mind adding a TODO.txt, reminding us what we want to deprecate/remove and in what version we want to do these things. It will start being hard to track otherwise.

@arokem

This comment has been minimized.

Member

arokem commented Dec 23, 2014

Other than that typo, and a TODO, to remind us when to get back to these things, looks ready to go!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 23, 2014

Should we create a new TODO file or make a section to the API changes page. I think the second idea is better. Let me try something.

Garyfallidis added some commits Dec 23, 2014

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 23, 2014

This should do the job. Let me know what you think.

@arokem

This comment has been minimized.

arokem commented on doc/api_changes.rst in bae5ba7 Dec 23, 2014

"The module peaks"?

This comment has been minimized.

Owner

Garyfallidis replied Dec 24, 2014

ok

@arokem

This comment has been minimized.

Member

arokem commented Dec 23, 2014

There's the API changes, that affect the user, and then there are changes we plan to make to the code (move files around, remove lines of code, etc.), which could go in here as well, or in another file (e.g. TODO.txt). We can also start writing the planned API changes for 0.9. For example: "dipy.align.aniso2iso will be removed, in favor of dipy.align.reslice", or something like that.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 24, 2014

A bit of feedback here. We had an offline discussion with Ariel and it seems a new TODO file is not necessary for now as we can use the issue tracker for now.

I wrote issues saying what will change in version 0.10.
And corrected the api changes doc here.

This PR should be ready to go.

arokem added a commit that referenced this pull request Dec 24, 2014

@arokem arokem merged commit 718891b into nipy:master Dec 24, 2014

1 check passed

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