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

NF: Convert between 4D DEC FA and 3D 24 bit representation. #1623

Merged
merged 5 commits into from Aug 31, 2018

Conversation

Projects
None yet
5 participants
@arokem
Copy link
Member

arokem commented Aug 23, 2018

CC: @jchoude

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Aug 23, 2018

Hello @arokem, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 31, 2018 at 15:10 Hours UTC
@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Aug 27, 2018

Hi @arokem

Thanks for that! Code is ok and works, but your test doesn't pass. Could have a look?

Cheers!

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Aug 27, 2018

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Aug 27, 2018

LGTM. Will wait until Travis is happy.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 27, 2018

Codecov Report

Merging #1623 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1623      +/-   ##
==========================================
- Coverage   87.33%   87.33%   -0.01%     
==========================================
  Files         246      246              
  Lines       32164    32171       +7     
  Branches     3497     3494       -3     
==========================================
+ Hits        28089    28095       +6     
  Misses       3242     3242              
- Partials      833      834       +1
Impacted Files Coverage Δ
dipy/io/utils.py 53.12% <100%> (+28.12%) ⬆️
dipy/io/tests/test_utils.py 100% <100%> (ø)
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️
dipy/tracking/tests/test_localtrack.py

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 a1e1f2e...4094100. Read the comment docs.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Aug 28, 2018

Seems like it's happy now.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Aug 28, 2018

Before merging, could you make pep8speaks happy @arokem ?

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Aug 31, 2018

@skoudoro I fixed the PEP8 comments. I'm waiting on Travis just to be absolutely sure.

Is there a way to ask PEP8 speaks to rerun?

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Aug 31, 2018

Thank you @jchoude. As you can see on top of this thread, it reruns automatically after a push and everything is good now

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Aug 31, 2018

Oh, nice, I didn't think it reran. Super!

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Aug 31, 2018

Thanks for fixing that!

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Aug 31, 2018

Does anyone understand what this is all about?

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Aug 31, 2018

No, and it is not the first time. This test passed 9/10. We have to understand why the decimal precision change sometimes on this test...
I just reran the build for this PR, it should be ok.

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Aug 31, 2018

Thanks for everything.

@jchoude jchoude merged commit f7e6828 into nipy:master Aug 31, 2018

3 checks passed

codecov/patch 100% of diff hit (target 87.33%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +12.66% compared to a1e1f2e
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