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: Implement NDC from Yeh2019 #3156

Merged
merged 7 commits into from Apr 1, 2024
Merged

ENH: Implement NDC from Yeh2019 #3156

merged 7 commits into from Apr 1, 2024

Conversation

mattcieslak
Copy link
Contributor

Adds a function to address #3154.

What would you recommend to add for a test?

@skoudoro skoudoro linked an issue Mar 27, 2024 that may be closed by this pull request
Copy link
Contributor

@arokem arokem left a comment

Choose a reason for hiding this comment

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

Looks good! Regarding testing, I'd suggest testing that correlation between a signal and itself is 1, maybe with different sets of b-vectors and b-values. Maybe also that this correlation doesn't change if one of the signals is scaled. For smoke testing, worth testing with different gradient schemes, with and without b=0, for example.

dipy/denoise/qc.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.40%. Comparing base (f765aa0) to head (f23a605).
Report is 4 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3156      +/-   ##
==========================================
+ Coverage   82.17%   82.40%   +0.23%     
==========================================
  Files         150      151       +1     
  Lines       21271    21306      +35     
  Branches     3413     3417       +4     
==========================================
+ Hits        17479    17558      +79     
+ Misses       2950     2908      -42     
+ Partials      842      840       -2     
Files Coverage Δ
dipy/stats/qc.py 100.00% <100.00%> (ø)

... and 8 files 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.

Also there is no automatic module discovery.

So, this new file needs to be added in meson.build.

Thank you for this. Personally, I need to read the paper

dipy/denoise/qc.py Outdated Show resolved Hide resolved
dipy/denoise/qc.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Mar 29, 2024

Hello @mattcieslak, Thank you for updating !

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

Comment last updated at 2024-04-01 13:34:08 UTC

@mattcieslak
Copy link
Contributor Author

should be good for review!

@arokem
Copy link
Contributor

arokem commented Mar 30, 2024

LGTM!

@ShreyasFadnavis
Copy link
Member

Looks great! Thanks for this @mattcieslak ! -- Not sure if denoise is the right module for it though?

I don't want people to miss this utility, just because they did not look in the right place.

Alternative modules could be stats?

@skoudoro
Copy link
Member

skoudoro commented Apr 1, 2024

Is there any additional comment @arokem @ShreyasFadnavis or @araikes??

I think this PR is ready to get in.

@arokem arokem merged commit 92bc506 into dipy:master Apr 1, 2024
30 checks passed
@skoudoro
Copy link
Member

skoudoro commented Apr 1, 2024

Thank you for this @mattcieslak !

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.

ENH: Add neighboring DWI correlation QC metric
5 participants