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

STYLE: Honor 'descoteaux'and 'tournier' SH basis naming. #1658

Merged
merged 1 commit into from Oct 30, 2018

Conversation

Projects
None yet
4 participants
@jhlegarreta
Copy link
Contributor

jhlegarreta commented Oct 27, 2018

Honor the authors in the naming for SH basis choice: replace and deprecate
fibernav and mrtrix and use descoteaux07 and tournier07 instead.

The change includes:

  • Using the author's name rather than the software/implementation's.
  • Testing that the deprecation warning is effectively exercised.
  • Adding the necesary references.
@jhlegarreta

This comment has been minimized.

Copy link
Contributor

jhlegarreta commented Oct 27, 2018

According to what was discussed in #1653.

And thus, closing that one.

A few additional comments as a follow up to the discussion in the previous PR:

  • I had an answer from Tournier to a mail about the SH basis and what MRtrix3 has. He said:
The old convention is described in appendix A1 of my 2007 paper.
The new one simply has a sqrt(2) factor in front of the m!=0 terms.
As far as I know, this convention is identical to  Maxime Descoteaux's
convention (equation 3 in his 2007 paper)

So I assume that in shm.py

The real harmonic $Y^m_n$ sampled at `theta` and `phi` as
implemented in mrtrix.  Warning: the basis is Tournier et al
2004 and 2007 is slightly different.

was not accurate/misworded. According to what Donald stated, I assume MRtrix3 has a basis which is not 2004 or 2007 (I guess it is 2007 with the correction factor). And this is effectively been pointed in the docs, Ch. 33, or the 2012 MRtrix reference pointed by @arokem if we want).

Since DIPY implements 2007 without the correction factor, just as originally published by Tournier, I changed the line above to:

Warning: the basis is Tournier et al 2007; 2004 is slightly different.

Some refs to mrtrix are still around in the function naming, e.g. here. I guess we are not willing to change them.

Thanks for the discussions in the previous PR.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:HonorAuthorsInSHBasisNaming branch 5 times, most recently from 71597dd to 524a376 Oct 27, 2018

STYLE: Honor 'descoteaux'and 'tournier' SH basis naming.
Honor the authors in the naming for SH basis choice: replace and deprecate
`fibernav` and `mrtrix` and use `descoteaux07` and `tournier07` instead.

The change includes:
- Using the author's name rather than the software/implementation's.
- Testing that the deprecation warning is effectively exercised.
- Adding the necesary references.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:HonorAuthorsInSHBasisNaming branch from 524a376 to 5840b09 Oct 28, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 28, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1658   +/-   ##
=========================================
  Coverage          ?   87.37%           
=========================================
  Files             ?      246           
  Lines             ?    32789           
  Branches          ?     3570           
=========================================
  Hits              ?    28648           
  Misses            ?     3278           
  Partials          ?      863
Impacted Files Coverage Δ
dipy/reconst/csdeconv.py 89.93% <ø> (ø)
dipy/direction/peaks.py 79.22% <ø> (ø)
dipy/reconst/tests/test_shm.py 98.95% <100%> (ø)
dipy/reconst/shm.py 86.6% <84.61%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5199af...5840b09. Read the comment docs.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Oct 29, 2018

I had an answer from Tournier to a mail about the SH basis...

Thank for this info. The code looks good so, I think we can merge it after another review.

Thanks @jhlegarreta

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Oct 30, 2018

Looks good to me. Merging.

@arokem arokem merged commit ae333ba into nipy:master Oct 30, 2018

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

@jhlegarreta jhlegarreta deleted the jhlegarreta:HonorAuthorsInSHBasisNaming branch Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment