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

RF - EuDX legacy code/test #1913

Merged
merged 7 commits into from Jul 25, 2019

Conversation

@gabknight
Copy link
Contributor

commented Jul 19, 2019

WIP waiting on: #1887, #1881

This addresses in part #1501

  • removed legacy EuDX code and tests
  • renamed PeaksAndMetricDirectionGetter to EuDXDirectionGetter

@arokem dipy/tracking/tests/test_life.py uses EuDX to generate streamlines, and then compare the LiFE results with the results of the Matlab implementation. LocalTracking does not produce precisely the same streamlines as EuDX (e.g. the last points of the streamlines are not kept anymore), making the test fail. To solve this, I propose we save the former EuDX streamlines and load the trk in this test. What do you think?

@gabknight gabknight force-pushed the gabknight:RF_local_tracking branch from 0aac259 to a8329e1 Jul 19, 2019

@gabknight

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

@arokem I went ahead and added the tracking file from the legacy EuDX code to dipy/data. This way the LiFE matlab_results and the streamlines will always match. Is that sounds OK?

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Hi @gabknight, Thank for this PR. I did almost the same on a local branch and I forgot to push and create a PR. My bad for duplicating the work and sorry for that. Otherwise, it looks good, I just need to tests your branch. Thank you again!

@gabknight

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

thx @skoudoro. No worries, please let me know if you did differently. I want to also move tracking/local to just tracking, and remove local in names, as discussed in the dipy meeting a few months ago. Did you do some work there? Otherwise, I will be finishing this PR in the coming days.

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Did you do some work there?

Not yet, because I would like to see #1912 and #1896 merged before

@gabknight

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

OK. Let's wait for merging #1912, #1896. I suggest we make a new PR for the RF of localtracking, and merge this one separately.

@gabknight gabknight changed the title WIP - RF - EuDX legacy code/test RF - EuDX legacy code/test Jul 23, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jul 24, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1913   +/-   ##
=========================================
  Coverage          ?   85.31%           
=========================================
  Files             ?      117           
  Lines             ?    14196           
  Branches          ?     2230           
=========================================
  Hits              ?    12111           
  Misses            ?     1581           
  Partials          ?      504
Impacted Files Coverage Δ
dipy/data/__init__.py 81.81% <100%> (ø)
dipy/direction/peaks.py 83.49% <100%> (ø)
@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Hi @gabknight, I fixed a conflict in your branch (test_propagation.py) as #1912 and #1896 are now merged. Will merge this PR today if the CIs come green.

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Thank you @gabknight, merging

@skoudoro skoudoro merged commit 03b2393 into nipy:master Jul 25, 2019

5 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
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
4 participants
You can’t perform that action at this time.