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

Update csdeconv.py #797

Merged
merged 1 commit into from Dec 24, 2015

Conversation

Projects
None yet
3 participants
@samuelstjean
Contributor

samuelstjean commented Nov 30, 2015

In [2]: iter = 8

In [3]: range(1, iter)
Out[3]: [1, 2, 3, 4, 5, 6, 7]

In [4]: len(range(1, iter))
Out[4]: 7

So now it should do 8 iterations by default, which is what I think is originally intended.

@@ -964,7 +964,7 @@ def recursive_response(gtab, data, mask=None, sh_order=8, peak_thr=0.01,
where_dwi = lazy_index(~gtab.b0s_mask)
response_p = np.ones(len(n))
for num_it in range(1, iter):
for num_it in range(iter):

This comment has been minimized.

@arokem

arokem Nov 30, 2015

Member

While you're at it, would you mind changing the name of this variable? iter is a key-word (a function) in the language.

This comment has been minimized.

@samuelstjean

samuelstjean Nov 30, 2015

Contributor

That would break the backward compatibility of the function (it's an input), including the script I am revamping which lead me to see this. Unless we add deprecation warning and everything to it it might break more stuff than it could fix.

This comment has been minimized.

@arokem

arokem Nov 30, 2015

Member

Yes. Good point. Sigh.

@arokem

This comment has been minimized.

Member

arokem commented Nov 30, 2015

@ChantalTax : could you please confirm this was your intention?

@arokem

This comment has been minimized.

Member

arokem commented Dec 10, 2015

Hey @ChantalTax - could you please confirm that this is indeed a bug fix?

@ChantalTax

This comment has been minimized.

Contributor

ChantalTax commented Dec 10, 2015

Hey! This seems indeed a bug fix, thanks!

@arokem

This comment has been minimized.

Member

arokem commented Dec 10, 2015

Great. Thanks @ChantalTax for confirming! And thanks @samuelstjean for noticing this! Is there any test you could write that would prevent this bug from recurring?

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Dec 10, 2015

er, not really, this just arose from the fact that matlab indexing starts from 1 and python indexing starts from 0 I think, it's just a small implementation detail overlook, not a real bug per se.

@arokem

This comment has been minimized.

Member

arokem commented Dec 10, 2015

I understand that. How did you run into this? Just by running the code, or
were you getting some odd results?
On Dec 10, 2015 8:29 AM, "Samuel St-Jean" notifications@github.com wrote:

er, not really, this just arose from the fact that matlab indexing starts
from 1 and python indexing starts from 0 I think, it's just a small
implementation detail overlook, not a real bug per se.


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

@arokem

This comment has been minimized.

Member

arokem commented Dec 10, 2015

Sorry, meant to say "just by reading the code"
On Dec 10, 2015 8:31 AM, "Ariel Rokem" arokem@gmail.com wrote:

I understand that. How did you run into this? Just by running the code, or
were you getting some odd results?
On Dec 10, 2015 8:29 AM, "Samuel St-Jean" notifications@github.com
wrote:

er, not really, this just arose from the fact that matlab indexing starts
from 1 and python indexing starts from 0 I think, it's just a small
implementation detail overlook, not a real bug per se.


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

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Dec 10, 2015

Reading the code, since it returns a whole object and the auto response returns a tuple, it was breaking a bunch of debug prints in the calling script I have to run CSD.

@arokem

This comment has been minimized.

Member

arokem commented Dec 10, 2015

OK -- thanks. I think this is fine to merge, and will wait until early next week to do so, in case anyone else has anything to add. Thanks for noticing this!

@arokem

This comment has been minimized.

Member

arokem commented Dec 24, 2015

Whoops - forgot to hit the merge button on this one!

🎄

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

@arokem arokem merged commit cb4d178 into nipy:master Dec 24, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@samuelstjean samuelstjean deleted the samuelstjean:patch-3 branch Jan 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment