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

NF - added pft min wm parameter #2821

Merged
merged 10 commits into from
Aug 28, 2023

Conversation

gabknight
Copy link
Contributor

This is the part 1 of 2 of refactoring PR #1627, adding a new parameter to PFT.

The new optional parameter (default value is 0), min_wm_pve_before_stopping, improves PFT starting in voxel with non-zero GM partial volume estimate (PVE). The 'ENDPOINT' streamline status is ignored until the tracking reaches a position with WM PVE > min_wm_pve_before_stopping. This is useful when seeding from the GM-WM interface or in subcortical GM, where tractography may stop prematurely in the GM. This parameter forces the tracking to reach the deep WM before stopping, or to reach voxels with WM PVE of 0.

@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #2821 (7758e0e) into master (ae6ab02) will increase coverage by 0.00%.
Report is 27 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2821   +/-   ##
=======================================
  Coverage   81.47%   81.48%           
=======================================
  Files         144      144           
  Lines       20063    20065    +2     
  Branches     3194     3195    +1     
=======================================
+ Hits        16347    16350    +3     
  Misses       2907     2907           
+ Partials      809      808    -1     
Files Changed Coverage Δ
dipy/workflows/tracking.py 96.59% <ø> (ø)
dipy/tracking/local_tracking.py 94.44% <100.00%> (+0.15%) ⬆️

... and 1 file with indirect coverage changes

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.

Overall, Looks good to me.

it would be great to update one of the tutorial to explain this parameter. It does not seems obvious to me when I read the code, but the PR description is super clear.

I do not know if you prefer to do that here or on a new PR. Otherwise, see below few comments

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

Hi @gabknight,

Can you address the conflict issue by a rebase and address my comment above. Thanks !

@gabknight gabknight force-pushed the NF_added_pft_min_wm_parameter branch from 8f868d0 to fccf910 Compare August 18, 2023 18:35
@gabknight
Copy link
Contributor Author

Hi @skoudoro,

Thank you. I addressed your comments and added a few lines in the PFT example.

@skoudoro
Copy link
Member

All looks good.

Before merging, I still need to check what is going on with test_horizon_events (https://github.com/dipy/dipy/actions/runs/5927644456/job/16189637170?pr=2821#step:9:3119).

The CI failed with this PR, and I do not know if it is related or not.

@skoudoro
Copy link
Member

Ok, I spent some times, and I can not reproduce it. Also, it does not seems related so I am going ahead and merge this PR.

Thank you @gabknight!

@skoudoro skoudoro merged commit 99030ce into dipy:master Aug 28, 2023
27 of 30 checks passed
@gabknight gabknight deleted the NF_added_pft_min_wm_parameter 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