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

[ENH] Asymmetric peak_directions #2751

Merged
merged 6 commits into from
Mar 29, 2023
Merged

Conversation

CHrlS98
Copy link
Contributor

@CHrlS98 CHrlS98 commented Mar 21, 2023

Hello,

Add a is_symmetric flag to dipy.direction.peaks.peak_directions to enable peak extraction on asymmetric ODFs. is_symmetric is True by default, so that the default behaviour does not change. When set to False, the sphere directions v and -v are considered distinct, such that it is possible to find a maxima along v but not along -v.

I simply removed the absolute value in remove_similar_vertices so that opposite sphere directions are not considered close to each other.

Have a good one!

@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #2751 (c877b96) into master (3667150) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head c877b96 differs from pull request most recent head 77fc969. Consider uploading reports for the commit 77fc969 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2751   +/-   ##
=======================================
  Coverage   83.90%   83.90%           
=======================================
  Files         132      132           
  Lines       18468    18471    +3     
  Branches     3018     3017    -1     
=======================================
+ Hits        15496    15499    +3     
  Misses       2227     2227           
  Partials      745      745           
Impacted Files Coverage Δ
dipy/direction/peaks.py 84.50% <ø> (ø)

... and 1 file with indirect coverage changes

Copy link
Contributor

@gabknight gabknight left a comment

Choose a reason for hiding this comment

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

Thanks @CHrlS98. Looks good to me. Just a few minor suggestions.

dipy/reconst/recspeed.pyx Outdated Show resolved Hide resolved
dipy/reconst/recspeed.pyx Show resolved Hide resolved
dipy/direction/peaks.py Outdated Show resolved Hide resolved
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.

Apart from the small comment below, all good to me.

Does someone else want to look at it before merging ???

dipy/direction/peaks.py Outdated Show resolved Hide resolved
@skoudoro
Copy link
Member

Thanks @CHrlS98, merging!

@skoudoro skoudoro merged commit 49cd0c8 into dipy:master Mar 29, 2023
@CHrlS98 CHrlS98 deleted the asym_peak_directions branch March 29, 2023 12:16
@CHrlS98 CHrlS98 restored the asym_peak_directions branch March 29, 2023 17:01
@CHrlS98 CHrlS98 deleted the asym_peak_directions branch May 16, 2023 15:10
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.

3 participants