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

BF - bad condition in maximum dg #1578

Merged
merged 2 commits into from Jun 29, 2018

Conversation

Projects
None yet
5 participants
@gabknight
Copy link
Contributor

gabknight commented Jun 28, 2018

this fix #1566.

Thanks @nilgoyette @jchoude

@nilgoyette

This comment has been minimized.

Copy link
Contributor

nilgoyette commented Jun 28, 2018

If all tests pass, lgtm.

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Jun 28, 2018

One check if failing, but it seems to not be related to this fix. The error is:

  z[index] = x
/home/travis/build/nipy/dipy/venv/lib/python3.5/site-packages/scipy/fftpack/basic.py:160: FutureWarning: Using a non-tuple sequence for multidimensional indexing is deprecated; use `arr[tuple(seq)]` instead of `arr[seq]`. In the future this will be interpreted as an array index, `arr[np.array(seq)]`, which will result either in an error or a different result.
@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Jun 28, 2018

Also LGTM.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jun 28, 2018

That warning is apparently a bug in the scipy pre-release version. See #1565

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jun 28, 2018

Could you please add a test that fails with current master, but passes when this fix is implemented?

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Jun 28, 2018

Ok. If no one else wants to check this micro PR, I could go ahead and merge it in a few days.

Anyone wants to test it?

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Jun 28, 2018

Oups my bad didn't see your last email. I'll let @gabknight and @nilgoyette coordinate on this.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 29, 2018

Codecov Report

Merging #1578 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1578      +/-   ##
==========================================
- Coverage   87.34%   87.33%   -0.02%     
==========================================
  Files         246      246              
  Lines       31918    31918              
  Branches     3471     3471              
==========================================
- Hits        27880    27875       -5     
- Misses       3212     3217       +5     
  Partials      826      826
Impacted Files Coverage Δ
dipy/reconst/mapmri.py 90.28% <0%> (-0.65%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8d2f17...ed88c5d. Read the comment docs.

@gabknight

This comment has been minimized.

Copy link
Contributor

gabknight commented Jun 29, 2018

I added the test. thx @arokem @nilgoyette @jchoude

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Jun 29, 2018

LGTM. Thanks @gabknight @nilgoyette

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jun 29, 2018

Yep - nice job - thanks for doing that! Per our conversation on #1579, I'm going to go ahead and merge this, despite the failure on the PRE=1 machine.

@arokem arokem merged commit bbb3e44 into nipy:master Jun 29, 2018

1 check failed

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

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

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