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' SH basis naming. #1653

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@jhlegarreta
Copy link
Contributor

jhlegarreta commented Oct 16, 2018

Honor 'descoteaux' naming for SH basis: replace and deprecate 'fibernav'.

Use the author's name rather than the software/implementation's.

@jhlegarreta

This comment has been minimized.

Copy link
Contributor

jhlegarreta commented Oct 16, 2018

The BIDS draft is already proposing this change.

The use of the former fibernav may need some deprecation cycle. Suggestions on this are welcome.

@skoudoro
Copy link
Member

skoudoro left a comment

Hi @jhlegarreta, Thank for this update! Concerning your question about deprecation cycle:

  1. You have to keep fibernav everywhere and just add descoteaux option
  2. Each time sph_harm_lookup.get(basis_type) is called, add something like:
if basis_type in 'fibernav': 
    warnings.warn("sh basis type `fibernav` is deprecated as of version" +
                  " 0.15 of Dipy and will be removed in a future " +
                  "version. Please use `descoteaux` instead",
                  DeprecationWarning)
Show resolved Hide resolved dipy/direction/peaks.py Outdated
Show resolved Hide resolved dipy/direction/peaks.py
Show resolved Hide resolved dipy/reconst/csdeconv.py Outdated
@@ -861,10 +862,10 @@ def sf_to_sh(sf, sphere, sh_order=4, basis_type=None, smooth=0.0):
sh_order : int, optional
Maximum SH order in the SH fit. For `sh_order`, there will be
``(sh_order + 1) * (sh_order_2) / 2`` SH coefficients (default 4).
basis_type : {None, 'mrtrix', 'fibernav'}
basis_type : {None, 'mrtrix', 'descoteaux'}

This comment has been minimized.

@skoudoro

skoudoro Oct 17, 2018

Member

same as above

Show resolved Hide resolved dipy/reconst/shm.py
@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Oct 17, 2018

Also a quick question: is there some place (in the doc, maybe?) where the different bases are explained, or a least a reference to the paper? I would guess somewhere in the sphm lookup code? Just to make sure what is defined.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Oct 17, 2018

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:HonorDescoteauxSHBasisNaming branch from 0e073ba to 11e6852 Oct 17, 2018

@jhlegarreta

This comment has been minimized.

Copy link
Contributor

jhlegarreta commented Oct 17, 2018

@skoudoro thanks for the review !

As for the docs, I can do it. Should this just go as a note in shm.py beside the dictionary sph_harm_lookup, or else/and should it be somewhere else in the docs/examples as well?

STYLE: Honor 'descoteaux' SH basis naming.
Honor 'descoteaux' naming for SH basis: deprecate 'fibernav'.

Use the author's name rather than the software/implementation's.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:HonorDescoteauxSHBasisNaming branch from 11e6852 to 8f53558 Oct 18, 2018

@jhlegarreta

This comment has been minimized.

Copy link
Contributor

jhlegarreta commented Oct 18, 2018

Test errors are unrelated; some configurations report success.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Oct 18, 2018

Yeah. We've been seeing these errors on other PRs in the last couple of days. See #1654

We're still trying to figure that one out.

As for documentation: I think that this issue would merit a page in the narrative documentation of the software. Or an entire example devoted just to an explanation of the differences between the basis sets (e.g., plotting the relationships between them, and how you might go between one and the other). It would be good to have something that we could point to whenever people ask us about this.

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Oct 19, 2018

Personnally, LGTM.

Should we go ahead with this, and have another PR for the documentation?

@jhlegarreta

This comment has been minimized.

Copy link
Contributor

jhlegarreta commented Oct 19, 2018

IMHO I think it's cleaner to do a separate topic for the documentation, since this kind of a feature/option-like addition, and in case of trouble, the documentation topic can be left aside/not blamed on. Just point where it should go, and I'll add that as soon as I can.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Oct 19, 2018

All PR's are blocked because of #1654. I think we should not merge it until we fix this issue and make Travis happy.

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Oct 19, 2018

Yes, sure, np for waiting, I just wanted to ensure that @jhlegarreta split the work in two branches.

@jhlegarreta

This comment has been minimized.

Copy link
Contributor

jhlegarreta commented Oct 20, 2018

As for the place where to put the references, I think desirable would be where the list is defined:

sph_harm_lookup = {None: real_sym_sh_basis,
                   "mrtrix": real_sym_sh_mrtrix,
                   "fibernav": real_sym_sh_basis}

But that does not make its way into the doc, so the alternative for MRtrix3 would be in the method real_sym_sh_mrtrix. I've asked to the MRtrix3 people which of the Tournier's (?) refs should be put, or else, whether we should directly ref to their documentation.

For, descoteaux it would be in the method real_sym_sh_basis, removing the link to the software and replacing by a ref to the paper

Descoteaux, M. , Angelino, E. , Fitzgibbons, S. and Deriche, R. (2006), Apparent diffusion
coefficients from high angular resolution diffusion imaging: Estimation and applications.
Magn. Reson. Med., 56: 395-410. doi:10.1002/mrm.20948

Please correct me if I'm wrong.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Oct 20, 2018

Why we do not do the same for mrtrix? removing the software and replace it by tournier and add the paper instead of the reference to their documentation?

@jhlegarreta

This comment has been minimized.

Copy link
Contributor

jhlegarreta commented Oct 20, 2018

That's something that crossed my mind when thinking about the documentation, but I did not bring it into the table because I thought that the MRtrix3 method was more likely to be changed over time/was better identified by the software, but it is definitely more consistent from our side to do the same for both cases.

So may be a new PR is then better if JC and Ariel are also for this change.

@arokem
Copy link
Member

arokem left a comment

I had a couple of comments on the code.

Regarding the naming of the mrtrix basis. What does the BIDS spec have to say about situations where the same author has proposed more than one basis set?

Regarding the documentation and references: I still think that it would be best handled by adding a page dedicated to this issue. In my opinion, we could certainly use more narrative documentation about issues of this sort. But it can definitely be handled in a separate PR, once we're done here.

``mrtrix`` for the MRtrix basis, and
``fibernav`` for the FiberNavigator basis
``descoteaux`` for the Descoteaux basis

This comment has been minimized.

@arokem

arokem Oct 20, 2018

Member

Need a reference here.

Also: should we remove 'fibernav' from here, since this is the one that is getting deprecated? If people have this in their code, we don't want to break it, but we also don't necessarily want to advertise this as an option, so people don't add it to new code they are writing.

basis : {None, 'mrtrix', 'fibernav'}
different spherical harmonic basis. None is the fibernav basis as well.
basis : {None, 'mrtrix', 'fibernav', 'descoteaux'}
different spherical harmonic basis. None is the descoteaux basis as well.

This comment has been minimized.

@arokem

arokem Oct 20, 2018

Member

Same comment here as above: remove the 'fibernav' option, and refer to the paper here.

@@ -861,10 +863,11 @@ def sf_to_sh(sf, sphere, sh_order=4, basis_type=None, smooth=0.0):
sh_order : int, optional
Maximum SH order in the SH fit. For `sh_order`, there will be
``(sh_order + 1) * (sh_order_2) / 2`` SH coefficients (default 4).
basis_type : {None, 'mrtrix', 'fibernav'}
basis_type : {None, 'mrtrix', 'fibernav', 'descoteaux'}

This comment has been minimized.

@arokem

arokem Oct 20, 2018

Member

And same comment here.

``fibernav`` for the FiberNavigator basis
``mrtrix`` for the MRtrix basis,
``fibernav`` (deprecated), and
``descoteaux`` for the Descoteaux basis
(default ``None``).

This comment has been minimized.

@arokem

arokem Oct 20, 2018

Member

I would add here that None implies 'descoteaux'

@@ -900,10 +908,11 @@ def sh_to_sf(sh, sphere, sh_order, basis_type=None):
sh_order : int, optional
Maximum SH order in the SH fit. For `sh_order`, there will be
``(sh_order + 1) * (sh_order_2) / 2`` SH coefficients (default 4).
basis_type : {None, 'mrtrix', 'fibernav'}
basis_type : {None, 'mrtrix', 'fibernav', 'descoteaux'}

This comment has been minimized.

@arokem

arokem Oct 20, 2018

Member

Same comments here as above

odf_sh = sf_to_sh(odf, sphere, 8, "fibernav")
odf2 = sh_to_sf(odf_sh, sphere, 8, "fibernav")
odf_sh = sf_to_sh(odf, sphere, 8, "descoteaux")
odf2 = sh_to_sf(odf_sh, sphere, 8, "descoteaux")
assert_array_almost_equal(odf, odf2, 2)

This comment has been minimized.

@arokem

arokem Oct 20, 2018

Member

You might want to add a test here for the deprecation warning.

See for example https://github.com/nipy/dipy/blob/master/dipy/reconst/tests/test_shm.py#L446 for how to catch a warning and test that it exists (in that example, we're checking that it doesn't exist).

@jhlegarreta

This comment has been minimized.

Copy link
Contributor

jhlegarreta commented Oct 22, 2018

@arokem thanks for the comments !

Regarding the naming of the mrtrix basis. What does the BIDS spec have to say about situations where the same author has proposed more than one basis set?

As for how BIDS foresees to handle multiple basis from the same author, as it may happen with Tournier's, I cannot say much: whether it is adding the year or some other appendix that allows to uniquely identify the basis in an informative way.

Also: should we remove 'fibernav' from here, since this is the one that is getting deprecated? If people have this in their code, we don't want to break it, but we also don't necessarily want to advertise this as an option, so people don't add it to new code they are writing.

Makes sense to me. I'm fine with whatever you decide is better.

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Oct 22, 2018

@arokem As for the naming of tournier / Mrtrix basis, Robert Smith (mrtrix core dev) commented that the Mrtrix-related bases should be mrtrix_0.2 and mrtrix3. I agree with that because of the difference in normalization between the 2 versions of the software. Else, we could have tournier and tournier_normalized, but for basic users it might be less clear.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Oct 22, 2018

I feel this change does not solve the problem. Neither makes things more clear.
I would really prefer if the name was related to the difference between the bases. Or if at least the name of the parameter would link to a specific paper and year. However, if we change this we should also change the mrtrix parameter.

Please also communicate before making changes in BIDS or otherwise concerning DIPY.
@jhlegarreta can you send me the link to the last BIDS update?

The parameters should be "descoteaux07" and "tournier07" rather than "fibernav" and "mrtrix". That I think clarifies the naming issue.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Oct 22, 2018

@jchoude for me also important is to have the year of the basis introduced if that is possible. So Tournier07 and Descoteaux06 etc. Mrtrix3 for example is not a solution because the basis can change many times while mrtrix3 is evolving. We have seen this before.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Oct 22, 2018

So, my suggestion is to have the authors' name and year were the basis was introduced.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Oct 22, 2018

And that I think solves most of our problems.

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Oct 22, 2018

@Garyfallidis I agree that the year might help. I have no strong opinion, I just think that having, somewhere easily accessible, a mapping telling people that the "old" Tournier basis == mrtrix 0.2, and the "new" Tournier is Mrtrix 3 would be quite helpful for users that don't know exactly the litterature.

Also, if I had to guess, I would bet that the Mrtrix 3 basis will not change anymore, since it's exactly the old Tournier basis with an added normalization. If they introduced some new fancy basis, I think it would have another different name.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Oct 22, 2018

Also @jhlegarreta - @jchoude has already sent me the link so no need to resend.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Oct 22, 2018

+1 to @Garyfallidis suggestions for naming (descoteaux07 and tournier07). According to that logic, the mrtrix3 basis would be tournier12, I believe (pointing to this paper: https://onlinelibrary.wiley.com/doi/abs/10.1002/ima.22005)

I think that a documentation page that has the mapping, explains the mapping and lays out all the namings should help users navigate these issues.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Oct 22, 2018

Test errors are unrelated; some configurations report success

it is fixed, you can rebase

@jhlegarreta

This comment has been minimized.

Copy link
Contributor

jhlegarreta commented Oct 23, 2018

Thanks for the discussion !

I will then submit the change in another topic (it is no longer only about fibernav) over the next few days: so descoteaux07 and tournier07. Adding the corresponding deprecation messages and testing them.

Will subsequently submit another PR with the doc if you let me know the best location for doing so.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Oct 23, 2018

@jhlegarreta

This comment has been minimized.

Copy link
Contributor

jhlegarreta commented Oct 24, 2018

Looks the right place to me 👍. Thanks Ariel. Will do these over the weekend.

@jhlegarreta jhlegarreta deleted the jhlegarreta:HonorDescoteauxSHBasisNaming branch Nov 4, 2018

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