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

Fix the relative SF threshold Issue #1583

Merged
merged 5 commits into from Aug 22, 2018

Conversation

Projects
None yet
5 participants
@frheault
Copy link

frheault commented Jul 5, 2018

Following the 4D interpolation and the fit, the amplitude of the values on the sphere are not always between 0 and 1, they can be very large or very small. Right after, a condition using a threshold (between 0 and 1) is performed.
The values on the sphere need to be scaled between 0 and 1 before applying the threshold.

As mentionned in #1582 , without this a simple linear scaling of the same FODf affect the tracking (ranging from a complete absence of streamlines to a broad tracking where most direction are acceptable)

@frheault frheault changed the title Fix #1582: Fix the relative SF threshold (Issue Fix the relative SF threshold (Issue Jul 5, 2018

@frheault frheault changed the title Fix the relative SF threshold (Issue Fix the relative SF threshold Issue Jul 5, 2018

for i in range(_len):
if pmf[i] < self.pmf_threshold:
if pmf[i] < self.pmf_threshold*max_pmf:

This comment has been minimized.

@jchoude

jchoude Jul 5, 2018

Contributor

You could factor the multiplication out of the loop.

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Jul 13, 2018

@gabknight Would you have a few minutes to look at this? I'm pretty confident in the identification of the issue and @frheault 's fix, but since you're more familiar than me in Dipy's tracking code, I'd like a second opinion.

@@ -101,8 +101,10 @@ cdef class BaseDirectionGetter(DirectionGetter):

pmf = self.pmf_gen.get_pmf_c(point)
_len = pmf.shape[0]
max_pmf = np.max(pmf)
relative_pmf_threshold = self.pmf_threshold*max_pmf

This comment has been minimized.

@gabknight

gabknight Jul 17, 2018

Contributor

Can you add double relative_pmf_threshold to the cdef declarations

I would also remove max_pmf and do:
relative_pmf_threshold = self.pmf_threshold * np.max(pmf)

@gabknight

This comment has been minimized.

Copy link
Contributor

gabknight commented Jul 17, 2018

Hi @frheault @jchoude,

Thanks for spotting this. We are currently using an absolute threshold not a relative threshold despite the documentation saying relative threshold. Your change fixes this.

  • Can you look into the failing tests? I suspect the toy PMFs used in the tests are not between 0 and 1. You can probably put the threshold to 0 for most tests, instead of the default 0.1.

  • Can you also add a test that fails on current master and passes with this PR? E.g. multiplying a pmf by 1 or 10, resulting in different ProbabilisticDirectionGetter directions with the same relative pmf threshold.

frheault
Modified the test as they were using absolute value instead of relati…
…ve, added a multiplicative factor to test
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 9, 2018

Codecov Report

Merging #1583 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1583      +/-   ##
==========================================
+ Coverage   87.33%   87.34%   +0.01%     
==========================================
  Files         246      246              
  Lines       31931    31811     -120     
  Branches     3471     3451      -20     
==========================================
- Hits        27886    27785     -101     
+ Misses       3218     3204      -14     
+ Partials      827      822       -5
Impacted Files Coverage Δ
dipy/tracking/local/tests/test_tracking.py 95.49% <100%> (ø) ⬆️
dipy/testing/__init__.py 81.81% <0%> (-1.52%) ⬇️
dipy/workflows/base.py 78.41% <0%> (-1.15%) ⬇️
dipy/viz/ui.py 88.89% <0%> (-0.06%) ⬇️
dipy/fixes/argparse.py 35.42% <0%> (-0.04%) ⬇️
dipy/tracking/life.py 97.8% <0%> (ø) ⬆️
dipy/tracking/tests/test_streamline.py 98.25% <0%> (ø) ⬆️
dipy/reconst/tests/test_shm.py 98.9% <0%> (ø) ⬆️
dipy/reconst/tests/test_dsi.py 97.67% <0%> (+0.02%) ⬆️
dipy/reconst/shm.py 86.68% <0%> (+0.04%) ⬆️
... and 8 more

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 c518e1b...08d9f4d. Read the comment docs.


pmf = self.pmf_gen.get_pmf_c(point)
_len = pmf.shape[0]

relative_pmf_threshold = self.pmf_threshold*np.max(pmf)

This comment has been minimized.

@jchoude

jchoude Aug 13, 2018

Contributor

Just to make sure, can it happen that pmf==0 everywhere?


pmf = self.pmf_gen.get_pmf_c(point)
_len = pmf.shape[0]

relative_pmf_threshold = self.pmf_threshold*np.max(pmf)

This comment has been minimized.

@jchoude

jchoude Aug 13, 2018

Contributor

Also, is it still relative if you scale it?

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Aug 16, 2018

LGTM.

Should we go ahead and merge this by monday morning, August 20th?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Aug 16, 2018

👍 from me!

@jchoude jchoude merged commit 1b90149 into nipy:master Aug 22, 2018

3 checks passed

codecov/patch 100% of diff hit (target 87.33%)
Details
codecov/project 87.34% (+0.01%) compared to c518e1b
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