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

"incompatible" import of peaks_from_model in your recent publication #328

Closed
kodiweera opened this issue Feb 19, 2014 · 10 comments
Closed

Comments

@kodiweera
Copy link

I was following code in your recent DiPy paper in frontiers (using recent 0.7.1 release). While in the section 6.5, I typed "from dipy.reconstruct.odf import peaks_from_model" which resulted in ImportError because in 0.6.0-564-gbdeca9e you refactored it into a separate peaks submodule. It would be nice if current 0.7.x stayed compatible with at least the code in the reference DiPy publication!

Thank you in advance!

@arokem
Copy link
Contributor

arokem commented Feb 19, 2014

Yep, that should definitely have been:

from dipy.reconst.peaks import peaks_from_model

This is still labeled "provisional pdf" on the Frontiers web-site. Any
chance we can still insert a correction?

On Wed, Feb 19, 2014 at 9:32 AM, kodiweera notifications@github.com wrote:

I was following code in your recent DiPy paper in frontiers (using recent
0.7.1 release). While in the section 6.5, I typed "from
dipy.reconstruct.odf import peaks_from_model" which resulted in ImportError
because in 0.6.0-564-gbdeca9e you refactored it into a separate peaks
submodule. It would be nice if current 0.7.x stayed compatible with at
least the code in the reference DiPy publication!

Thank you in advance!


Reply to this email directly or view it on GitHubhttps://github.com//issues/328
.

@Garyfallidis
Copy link
Contributor

Maybe there is still time if we are lucky. Just sent them a message. Thank you @kodiweera

@MrBago
Copy link
Contributor

MrBago commented Feb 19, 2014

How do you guys feel about importing peaks_from_model into the
dipy.recost.odf namespace so that we don't have this problem going forward?
We'd need to make another bugfix release and anyone with 7.1 wouldn't see
the change, but anyone who download the release after the fix wouldn't hit
this issue.

Bago

On Wed, Feb 19, 2014 at 10:14 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Maybe there is still time if we are lucky. Just sent them a message. Thank
you @kodiweera https://github.com/kodiweera

Reply to this email directly or view it on GitHubhttps://github.com//issues/328#issuecomment-35529557
.

@kodiweera
Copy link
Author

There are few more typos in it. so far I encountered the following.
On the page 7, you have
ten_fit = ten.fit(data,mask)

For consistency, it should be
ten_fit = ten_model.fit(data,mask)

And also, in the page 7, you have
cfa = color_fa(fa, tenfit.evecs)

For consistency, it should be
cfa = color_fa(fa, ten_fit.evecs)

will let you know if there are more typos!

@Garyfallidis
Copy link
Contributor

Thank you @kodiweera but from what I see now the frontiers website does not allow any more changes on our side. Let's not drive ourselves crazy here. There will always be some typos here and there. It's okay.

@MrBago I really don't think that what you suggest is necessary to do. Anyone having problems with peaks_from_model the first thing to do would be to google the function online or look at the tutorials and if you do that you will find the correct version. Let's move forward guys.

@MrBago
Copy link
Contributor

MrBago commented Feb 19, 2014

We all know that dipy is going to explode with this paper :) so I just want
to make sure we haven't broken compatibility before the official version of
the paper is even out. If we can still fix the paper, that's obviously
preferable.

Bago

On Wed, Feb 19, 2014 at 11:46 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Thank you @kodiweera https://github.com/kodiweera but from what I see
now the frontiers website does not allow any more changes on our side.
Let's not drive ourselves crazy here. There will always be some typos here
and there. It's okay.

@MrBago https://github.com/MrBago I really don't think that what you
suggest is necessary to do. Anyone having problems with peaks_from_model
the first thing to do would be to google the function online or look at the
tutorials and if you do that you will find the correct version. Let's move
forward guys.

Reply to this email directly or view it on GitHubhttps://github.com//issues/328#issuecomment-35539801
.

@Garyfallidis
Copy link
Contributor

I think the change to peaks was the last comment which I was allowed to do in Frontier's website for the Dipy paper. Fingers' crossed I hope the Frontiers' typesetters will make the change. Cannot say for sure yet. I will let you know if they get back to me.

@Garyfallidis
Copy link
Contributor

Okay, I just sent an e-mail directly to Frontiers' production office with all the corrections suggested by @kodiweera. I hope it goes through. Thanx again @kodiweera.

@arokem
Copy link
Contributor

arokem commented Feb 20, 2014

Seems like it worked! Thanks @kodiweera, and thanks @Garyfallidis for
getting there on time...

On Wed, Feb 19, 2014 at 12:51 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Okay, I just sent an e-mail directly to Frontiers' production office with
all the corrections suggested by @kodiweera https://github.com/kodiweera.
I hope it goes through. Thanx again @kodiweerahttps://github.com/kodiweera
.


Reply to this email directly or view it on GitHubhttps://github.com//issues/328#issuecomment-35546904
.

@skoudoro
Copy link
Member

It seems to be done, I close

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

No branches or pull requests

5 participants