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

Workflows - Adding PFT, probabilistic, closestpeaks tracking #1640

Merged
merged 24 commits into from Feb 4, 2019

Conversation

@skoudoro
Copy link
Member

commented Sep 14, 2018

The aim of this PR is :

  • To update Local tracking workflow.
  • To Allow different Direction Getter
  • To add PFT workflow

Things to do:

  • Add test for PFT workflows
  • Update bin command line

@skoudoro skoudoro requested a review from Garyfallidis Sep 14, 2018

@skoudoro skoudoro requested a review from gabknight Sep 16, 2018

@skoudoro skoudoro changed the title [WIP] Workflows - Adding PFT, probabilistic, closestpeaks Workflows - Adding PFT, probabilistic, closestpeaks Sep 16, 2018

@skoudoro skoudoro changed the title Workflows - Adding PFT, probabilistic, closestpeaks Workflows - Adding PFT, probabilistic, closestpeaks tracking Sep 16, 2018

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2018

@gabknight, any thought?

@gabknight

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2018

Hi @skoudoro,

That looks good, thanks for doing this. See below some quick thought, I will review the code later this week.

I'm not a fan of using the PAM file as input. The main advantage is that you can have this single file for all dg, but the PAM files are very inconvenient to generate from outside dipy. I would rather have workflows that take as input peaks file or PMF/ODF files or SH files. This way, users that developed their own peak extraction method or FOD estimation can still use the workflows for the tracking. Moreover, PFT tracking only works with the ProbabilisticDirectionGetter (PMF/ODF or SH), so no need for the peaks. There are also no guarantee that the PAM file contains the ODF or SH.
What do you think?

Some minor things:

  • Both workflows miss the pmf_threshold and max_angle parameter for the dg.

  • use_sh sh_strategy are not useful for PFTrackingPAMFlow as probabilistic tracking is mandatory for PFT. This is actually not enforced in the code (just in the doc) , but if the dg is not probabilistic the output of PFT will be the same as LocalTracking.

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2018

Thank you really much for this feedback @gabknight!

I'm not a fan of using the PAM file as input

I agree with your analysis. We need to have this 2 options, so I think I will add another workflow which manages the second case

Both workflows miss the pmf_threshold and max_angle parameter for the dg

good catch, I will update that point

use_sh sh_strategy are not useful for PFTrackingPAMFlow

Maybe we should enforce in the code... This is a point to discuss

@codecov-io

This comment has been minimized.

Copy link

commented Oct 15, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1640   +/-   ##
=========================================
  Coverage          ?   84.27%           
=========================================
  Files             ?      115           
  Lines             ?    13606           
  Branches          ?     2144           
=========================================
  Hits              ?    11466           
  Misses            ?     1643           
  Partials          ?      497
Impacted Files Coverage Δ
dipy/data/__init__.py 82.05% <ø> (ø)
dipy/data/fetcher.py 33.42% <60%> (ø)
dipy/workflows/tracking.py 95.94% <93.87%> (ø)

@skoudoro skoudoro force-pushed the skoudoro:workflows-tracking branch 2 times, most recently from 4c1cb0b to a3b6153 Oct 16, 2018

@gabknight
Copy link
Contributor

left a comment

Looks good to me. See my comment for a minor change in the selection of tracking algorithm.

I also suggest to rename the workflows as following:

  • dipy_lf_track -> dipy_track_local
  • dipy_pf_track -> dipy_track_pft

As for dipy_fit_* this would group all tracking workflows together : dipy_track_*.

Thanks for putting this together,

@@ -32,11 +71,10 @@ def _core_run(self, stopping_path, stopping_thr, seeding_path,
direction_getter = pam

if use_sh:

This comment has been minimized.

Copy link
@gabknight

gabknight Jan 17, 2019

Contributor

There is a problem here. If the use set --sh_strategy prob without `--use_sh" the output is deterministic using the pam.

My suggestion is to remove --use_sh and rename --sh_strategy to --tracking_method or --tracking_strategy, with the following opton:

  • EuDX -> direction_getter = pam
  • 'deterministic' -> direction_getter = DeterministicMaximumDirectionGetter.from_shcoeff(pam.shm_coeff, ...)
  • closetpeaks -> direction_getter = ClosestPeakDirectionGetter.from_shcoeff(pam.shm_coeff, ...)
    -probabilistic -> direction_getter = ProbabilisticDirectionGetter.from_shcoeff(pam.shm_coeff, ...)

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jan 17, 2019

Author Member

There is a problem here. If the use set --sh_strategy prob without `--use_sh" the output is deterministic using the pam

Good catch! Thank you. fixed in bd4c305

@skoudoro skoudoro force-pushed the skoudoro:workflows-tracking branch from a3b6153 to 7f6fe23 Jan 17, 2019

@pep8speaks

This comment has been minimized.

Copy link

commented Jan 17, 2019

Hello @skoudoro, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 01, 2019 at 16:53 Hours UTC
@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

Thank you for your feedback @gabknight! Your naming convention makes sense so I switched.

@gabknight
Copy link
Contributor

left a comment

thx @skoudoro for the quick changes :). I have a few other minor comments. It will then be ready to merge.

@@ -48,14 +48,17 @@ def _get_direction_getter(self, strategy_name):
elif strategy_name.lower() in ["closestpeaks", "cp"]:
msg = "ClosestPeaks"
direction_getter = ClosestPeakDirectionGetter
elif strategy_name.lower() in ["eudx", ""] and 'pam' in kwargs.keys():

This comment has been minimized.

Copy link
@gabknight

gabknight Jan 18, 2019

Contributor

I think "" should be removed. Otherwise "eudx" will be the default algorithm instead of deterministic, no?

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jan 18, 2019

Author Member

I do not think eudx will be selected as default since deterministic is the default value on run function. But, I agree to remove it and avoid confusion

Select direction getter strategy:
- "eudx" (Use spherical harmonics saved in peaks)

This comment has been minimized.

Copy link
@gabknight

gabknight Jan 18, 2019

Contributor
  • "eudx" (Uses the peaks saved in the pam_files)
Select direction getter strategy:
- "eudx" (Use spherical harmonics saved in peaks)
- "deterministic" or "det" for a deterministic tracking (default)

This comment has been minimized.

Copy link
@gabknight

gabknight Jan 18, 2019

Contributor
  • "deterministic" or "det" for a deterministic tracking (Uses the sh saved in the pam_files, default)
dipy/workflows/tracking.py Show resolved Hide resolved
dipy/workflows/tracking.py Show resolved Hide resolved
max_angle=max_angle,
sphere=pam.sphere,
pmf_threshold=pmf_threshold)
direction_getter = dg.from_shcoeff(pam.shm_coeff,

This comment has been minimized.

Copy link
@gabknight

gabknight Jan 18, 2019

Contributor

there is a bug here using EuDX. The pam object doesn't have the from_shcoeff(.) method as it derives from the DirectionGetter class rather then PmfGenDirectionGetter. I think you can solve this by moving

direction_getter = dg.from_shcoeff(pam.shm_coeff,
                                   max_angle=max_angle,
                                   sphere=pam.sphere,
                                   pmf_threshold=pmf_threshold)

inside the function _get_direction_getter(.), with e.g. dg = DeterministicMaximumDirectionGetter.from_shcoeff(pam.shm_coeff, max_angle=max_angle, sphere=pam.sphere, pmf_threshold=pmf_threshold).

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jan 18, 2019

Author Member

yes, I saw that, fixed on 0f9ac03

class LocalFiberTrackingPAMFlow(Workflow):
@classmethod
def get_short_name(cls):
return 'lf_track'

This comment has been minimized.

Copy link
@gabknight

gabknight Jan 18, 2019

Contributor

to be consistent with the workflow name change, I guess this should be 'local_track'?

@gabknight
Copy link
Contributor

left a comment

LGTM. thx @skoudoro

@Garyfallidis do you want to review this PR? Otherwise, I will ahead and merge it at the end of the week.

@skoudoro skoudoro force-pushed the skoudoro:workflows-tracking branch from 1162866 to 9606c64 Feb 1, 2019

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

Rebased again, after the #1652 merge.

@gabknight gabknight merged commit 52f6e37 into nipy:master Feb 4, 2019

4 checks passed

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

@skoudoro skoudoro deleted the skoudoro:workflows-tracking branch Feb 7, 2019

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.