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

BF - initial backward orientation of local tracking #2806

Merged
merged 20 commits into from
Aug 18, 2023

Conversation

gabknight
Copy link
Contributor

@gabknight gabknight commented May 5, 2023

This address issue #2805.

  • Reverts changes in dipy/tracking/localtrack.pyx from 045a1ee
  • fix a minor infrequent bug in probabilistic tracking where the forward and backward direction could be sample in such a way it is bigger that max_angle. This was fixed by changing the initial direction to the true first direction selected in the forward segment, instead of the tentative initial direction obtained from the peaks.

@gabknight gabknight changed the title BF - backward orientation of local tracking BF - initial backward orientation of local tracking May 5, 2023
@skoudoro
Copy link
Member

skoudoro commented May 5, 2023

Can you create a test to make sure we detect it next time ?

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #2806 (359ba73) into master (aa39ed3) will increase coverage by 0.00%.
Report is 50 commits behind head on master.
The diff coverage is 77.77%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2806   +/-   ##
=======================================
  Coverage   81.47%   81.47%           
=======================================
  Files         144      144           
  Lines       20054    20063    +9     
  Branches     3192     3194    +2     
=======================================
+ Hits        16339    16347    +8     
- Misses       2906     2907    +1     
  Partials      809      809           
Files Changed Coverage Δ
dipy/tracking/local_tracking.py 94.28% <77.77%> (-1.60%) ⬇️

... and 2 files with indirect coverage changes

@gabknight
Copy link
Contributor Author

gabknight commented May 8, 2023

I added the tests, which identify another minor bug, again at the junction of the forward and backward streamline segments, for probabilistic tracking. For the backward segment, I changed the tentative initial direction (peaks) to the true initial orientation (vector connecting the first and second points of the forward segment.). Since orientations are always sphere vertices for PmfGenDirectionGetter, I needed calling sphere.find_closest() with the updated initial direction, when querying the dictionary of neighbouring orientations. I did it through a try-except to remove the check at each iteration, what do you think?

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gabknight,

Thank you for fixing this bug. See below my comments. I will look for an alternative to try/except clause

dipy/direction/probabilistic_direction_getter.pyx Outdated Show resolved Hide resolved
dipy/direction/probabilistic_direction_getter.pyx Outdated Show resolved Hide resolved
dipy/tracking/local_tracking.py Outdated Show resolved Hide resolved
dipy/tracking/localtrack.pyx Outdated Show resolved Hide resolved
@skoudoro
Copy link
Member

Thank you for the update @gabknight!

it seems there is still some issues with Windows as you can see below

FAILED tracking/tests/test_tracking.py::test_tracking_max_angle - AssertionError
FAILED tracking/tests/test_tracking.py::test_particle_filtering_tractography - AssertionError: 
Arrays are not almost equal to 7 decimals
 ACTUAL: 0.0
 DESIRED: 0.2

I suppose srand have different result between windows and linux. you might need to find an alternative for that.

@gabknight
Copy link
Contributor Author

I created issue #2867 to address the random fct call problem in another PR.

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @gabknight,

Looks good! Thanks for this work. merging

@skoudoro skoudoro merged commit ae6ab02 into dipy:master Aug 18, 2023
24 of 27 checks passed
@gabknight gabknight deleted the BF_initial_direction branch December 8, 2023 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants