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

Fixes 238, by requiring vol_idx input with 4D images. #1821

Merged
merged 9 commits into from May 9, 2019

Conversation

@arokem
Copy link
Member

commented May 3, 2019

See #238

@skoudoro

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Great, Thank you for that! Waiting for Travis...

@skoudoro

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Great! Thank you for this fix! As you can see here it seems you need to update some workflow test. After that, I will go ahead and merge it

@arokem

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Yeah. I saw that. Will fix!

@arokem arokem force-pushed the arokem:fix-238 branch from ddd717d to 7c6688b May 6, 2019

@arokem

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

OK. I think that this is ready to be reviewed. I believe that remaining CI failures are unrelated.

@skoudoro
Copy link
Member

left a comment

Overall, it looks good to me. You just need to update the workflows documentation (currently wrong for vol_idx). After that, it can be merged.

Thank you @arokem!

@codecov-io

This comment has been minimized.

Copy link

commented May 7, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@2ce4059). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1821   +/-   ##
=========================================
  Coverage          ?   83.59%           
=========================================
  Files             ?      118           
  Lines             ?    14486           
  Branches          ?     2283           
=========================================
  Hits              ?    12109           
  Misses            ?     1851           
  Partials          ?      526
Impacted Files Coverage Δ
dipy/workflows/stats.py 92% <100%> (ø)
dipy/segment/mask.py 80.55% <100%> (ø)
dipy/workflows/segment.py 72.44% <100%> (ø)

@skoudoro skoudoro added this to the 1.0 milestone May 7, 2019

@arokem

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

Is this the doc you meant?

@skoudoro

This comment has been minimized.

Copy link
Member

commented May 7, 2019

yes, perfect 👍

@arokem arokem force-pushed the arokem:fix-238 branch from c3ba848 to d0f3dc7 May 7, 2019

@skoudoro

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Hi @arokem, Do you understand the Travis error? I am surprised since everything is good at #1775

@arokem

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Not really, and I don't understand why it passed there, but not here...

But maybe it has to do with this line: https://github.com/nipy/dipy/blob/master/.travis.yml#L64 ?

Why are we pointing to python2 here?

@skoudoro

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Ok, I see, good catch. It was mainly because of VTK. But we do not need that anymore since we install VTK via Pypi.

@arokem

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

I can try fixing that here.

.travis.yml Outdated
- TEST_WITH_XVFB=true
- DEPENDS="$DEPENDS scikit_learn fury"
- MESA_GL_VERSION_OVERRIDE=3.3

This comment has been minimized.

Copy link
@skoudoro

skoudoro May 8, 2019

Member

sorry, you have to keep: MESA_GL_VERSION_OVERRIDE=3.3, LIBGL_ALWAYS_INDIRECT=y, DEPENDS="$DEPENDS scikit_learn fury" but remove this line

@skoudoro

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Thank you @arokem, merging

@skoudoro skoudoro merged commit 0431080 into nipy:master May 9, 2019

4 checks passed

codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.