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

RF: Move the peaks module from dipy.reconst to dipy.directions. #770

Merged
merged 5 commits into from Nov 20, 2015

Conversation

arokem
Copy link
Contributor

@arokem arokem commented Nov 13, 2015

Addresses #517

@MrBago
Copy link
Contributor

MrBago commented Nov 13, 2015

Shouldn't this get moved to dipy/direction and relevant imports be adjusted?

@arokem
Copy link
Contributor Author

arokem commented Nov 15, 2015

Clearly this by itself is no good :-)

@arokem arokem changed the title RF: Remove the peaks module from dipy.reconst. RF: Move the peaks module from dipy.reconst to dipy.directions. Nov 15, 2015
@arokem
Copy link
Contributor Author

arokem commented Nov 16, 2015

How does this look @MrBago ?

@samuelstjean
Copy link
Contributor

Can you add a deprecation warning and import the new module in the old one for compatibility with old stuff?

…ning.

To be removed completely on the next cycle.
@arokem
Copy link
Contributor Author

arokem commented Nov 16, 2015

How's that @samuelstjean ?

@Garyfallidis
Copy link
Contributor

@MrBago can you merge this PR if you think it is okay? We are in release mode right now and we would appreciate some quick feedback so we can move on. Thx in advance.

@MrBago
Copy link
Contributor

MrBago commented Nov 19, 2015

This looks good to me. My only question is about the DeprecationWarning, these are silenced by default so most people will not see them. Do we want something stronger? I feel like this question has come up before so if we settled on decision, I'm sorry for raising it again.

@Garyfallidis
Copy link
Contributor

I agree that this is an important discussion and I am not sure we had a final decision on this. But because this is a generic design issue and not related with this PR I would suggest to move on with the release and then open an issue or a discussion in gitter to figure out the best warning strategy. Sounds good?

MrBago added a commit that referenced this pull request Nov 20, 2015
RF: Move the peaks module from dipy.reconst to dipy.directions.
@MrBago MrBago merged commit 59c1c6a into dipy:master Nov 20, 2015
@MrBago
Copy link
Contributor

MrBago commented Nov 20, 2015

K, I'm merging this. I brought this up partially because we're already using UserWarnings for this kind of thing in some places, /home/bagrata/projects/dipy/dipy/align/aniso2iso.py.

@Garyfallidis
Copy link
Contributor

Good reminder. Write an issue please so we can keep track of that.

@Garyfallidis
Copy link
Contributor

We should clearly resolve this problem by 0.11 release.

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.

None yet

4 participants