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

Faster dti odf #1064

Merged
merged 8 commits into from Oct 31, 2016

Conversation

Projects
None yet
4 participants
@arokem
Member

arokem commented May 27, 2016

Speeds up DTI odf calculation, by calculating it only over regions of the volume that have non-zero eigen-values.

@arokem

This comment has been minimized.

Member

arokem commented May 27, 2016

This saves a little bit of time for the case in scratch/profile_dti.py:

this branch: 48.5043 s
master: 57.4576 s

But should save substantially more time when a mask is used.

@arokem

This comment has been minimized.

Member

arokem commented Oct 21, 2016

This one is also an easy review: <40 lines of changed code for a 12% speedup for something that some users probably do 20 times a day. Any one want to take a look?

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 21, 2016

LGTM. It avoids divisions by zero and simplifies the code.

@arokem arokem force-pushed the arokem:faster-dti-odf branch from 1156d3a to 767bb04 Oct 21, 2016

@arokem

This comment has been minimized.

Member

arokem commented Oct 21, 2016

Thanks for taking a look. Now also rebased for ease of merging.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 26, 2016

@arokem : it seems the rebase went wrong?

@arokem arokem force-pushed the arokem:faster-dti-odf branch from 767bb04 to 67b349f Oct 26, 2016

@arokem

This comment has been minimized.

Member

arokem commented Oct 26, 2016

Looks like Travis was having a hard time connecting to Github. I just repushed that last commit.

@codecov-io

This comment has been minimized.

codecov-io commented Oct 26, 2016

Current coverage is 84.61% (diff: 100%)

Merging #1064 into master will increase coverage by 3.74%

@@             master      #1064   diff @@
==========================================
  Files           217        219      +2   
  Lines         24593      24879    +286   
  Methods           0          0           
  Messages          0          0           
  Branches       2491       2515     +24   
==========================================
+ Hits          19888      21051   +1163   
+ Misses         4194       3199    -995   
- Partials        511        629    +118   

Powered by Codecov. Last update da31980...58532e6

@coveralls

This comment has been minimized.

coveralls commented Oct 26, 2016

Coverage Status

Coverage decreased (-0.01%) to 82.925% when pulling 67b349f on arokem:faster-dti-odf into da31980 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Oct 26, 2016

Coverage Status

Coverage decreased (-0.01%) to 82.925% when pulling 67b349f on arokem:faster-dti-odf into da31980 on nipy:master.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 26, 2016

@arokem: Should I wait for you to add a small test that actually uses the mask (see codecov)?

@arokem

This comment has been minimized.

Member

arokem commented Oct 27, 2016

oh yeah - thats no good. please wait. ill get to it in the weekend, i hope

On Oct 26, 2016 2:19 PM, "Marc-Alexandre Côté" notifications@github.com
wrote:

@arokem https://github.com/arokem: Should I wait for you to add a small
test that actually uses the mask (see codecov)?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1064 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHPNhnpdK5Ml7mFkjmr2woxUPyGJwiXks5q35mngaJpZM4Io3Kv
.

@coveralls

This comment has been minimized.

coveralls commented Oct 29, 2016

Coverage Status

Coverage increased (+4.2%) to 87.121% when pulling 03ebcd5 on arokem:faster-dti-odf into da31980 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Oct 31, 2016

How about this? On my machine, the dti module is currently at 99% coverage.

@coveralls

This comment has been minimized.

coveralls commented Oct 31, 2016

Coverage Status

Coverage increased (+4.2%) to 87.142% when pulling 615283e on arokem:faster-dti-odf into da31980 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Oct 31, 2016

Coverage Status

Coverage increased (+4.2%) to 87.142% when pulling 58532e6 on arokem:faster-dti-odf into da31980 on nipy:master.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 31, 2016

Excellent!

@MarcCote MarcCote merged commit 710fdc4 into nipy:master Oct 31, 2016

4 checks passed

codecov/patch 100% of diff hit (target 80.86%)
Details
codecov/project 84.61% (+3.74%) compared to da31980
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+4.2%) to 87.142%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment