[RF] Fix spherical harmonic terminology swap #3086

itellaetxe · 2024-02-28T13:54:31Z

Summary

For functions related to spherical harmonics, the terminology "degree" and "order" is often used backwards in docstrings and variable names. This PR ensures consistency when referring to "degree" and "order" in the code. Now instead of "degree", we use "phase factor".
Closes Issue #2970

Main changes

Standardized the symbols for the spherical harmonic concepts of order and phase factor with l_value and m_value, respectively. In case these are arrays, we use l_values and m_values to be more informative of the type of the variable.
I followed the indications given by @arokem and @skoudoro, regarding the new namings. For a spherical harmonic $Y_{\ell}^m$ -> $\ell$ is referred to as the order and $m$ as phase factor.
sh_order is now sh_order_max.
Included the a note in the theory rst file sh_basis.rst explaining our convention choice.
Fixed a couple of bad namings I overlooked the first time I opened the PR.
Passed tests locally.
Rebased my branch onto master.

pep8speaks · 2024-02-28T13:54:58Z

Hello @itellaetxe, Thank you for updating !

In the file dipy/reconst/csdeconv.py:

Line 161:81: E501 line too long (86 > 80 characters)
Line 806:1: E302 expected 2 blank lines, found 1
Line 1081:1: E303 too many blank lines (3)

In the file dipy/reconst/forecast.py:

Line 84:75: W291 trailing whitespace
Line 450:1: E302 expected 2 blank lines, found 1
Line 457:9: E741 ambiguous variable name 'l'
Line 464:1: E302 expected 2 blank lines, found 1
Line 482:1: E302 expected 2 blank lines, found 1

In the file dipy/reconst/mapmri.py:

Line 1530:48: W291 trailing whitespace

In the file dipy/reconst/mcsd.py:

Line 24:1: E302 expected 2 blank lines, found 1
Line 100:1: E302 expected 2 blank lines, found 1
Line 113:9: E125 continuation line with same indent as next logical line
Line 438:1: E302 expected 2 blank lines, found 1

In the file dipy/reconst/shm.py:

Line 86:29: E231 missing whitespace after ','
Line 86:81: E501 line too long (83 > 80 characters)
Line 133:29: E231 missing whitespace after ','
Line 133:81: E501 line too long (83 > 80 characters)
Line 173:29: E231 missing whitespace after ','
Line 173:81: E501 line too long (83 > 80 characters)
Line 219:81: E501 line too long (102 > 80 characters)
Line 228:29: E231 missing whitespace after ','
Line 228:81: E501 line too long (83 > 80 characters)
Line 261:81: E501 line too long (85 > 80 characters)
Line 264:29: E231 missing whitespace after ','
Line 264:81: E501 line too long (83 > 80 characters)
Line 320:29: E231 missing whitespace after ','
Line 320:81: E501 line too long (83 > 80 characters)
Line 371:1: E302 expected 2 blank lines, found 1
Line 426:81: E501 line too long (81 > 80 characters)
Line 430:1: E302 expected 2 blank lines, found 1
Line 590:1: E302 expected 2 blank lines, found 1
Line 1135:1: E302 expected 2 blank lines, found 1
Line 1200:1: E302 expected 2 blank lines, found 1
Line 1261:1: E302 expected 2 blank lines, found 1
Line 1646:81: E501 line too long (84 > 80 characters)

In the file dipy/tracking/tests/test_tracking.py:

Line 709:81: E501 line too long (83 > 80 characters)

Comment last updated at 2024-03-08 19:24:55 UTC

itellaetxe · 2024-02-28T13:56:50Z

My linter was not showing me the E501 problem. Fixing it. Sorry

skoudoro · 2024-02-28T16:46:45Z

Hi @itellaetxe,

We fixed the issue in master, Can you rebase this branch or merge master to this branch?

Thanks

CHrlS98 · 2024-02-28T21:00:55Z

Hi @skoudoro @itellaetxe @ebrahimebrahim,
The terminology swap was introduced in a previous PR of mine (#2206). We also briefly discussed the swap here https://github.com/dipy/dipy/pull/2206#pullrequestreview-484290448. However I now realize that what we called the "literature" at the time is actually limited to the diffusion MR literature and from a mathematician's/physicist's point-of-view it might be wrong. Still, both bases we support in DIPY (descoteaux07 and tournier07) use the term order to describe $\ell$ in the cited papers.

If we do proceed with this change we might want to add a note somewhere in the doc clearly explaining the issue. It looks like the dMRI community have settled on the terminology order and all the papers I came across using CSD reconstruction report the maximum SH order when they talk about $\ell_{max}$ (although it is true that the maximum SH order and degrees will be equal).

This is very confusing and unclear to me why there is a swap between the dMRI literature and wikipedia. I'll also tag @karanphil here who might have something to add.

karanphil · 2024-02-28T21:11:51Z

Hi @skoudoro @itellaetxe @ebrahimebrahim, The terminology swap was introduced in a previous PR of mine (#2206). We also briefly discussed the swap here #2206 (review). However I now realize that what we called the "literature" at the time is actually limited to the diffusion MR literature and from a mathematician's/physicist's point-of-view it might be wrong. Still, both bases we support in DIPY (descoteaux07 and tournier07) use the term order to describe ℓ in the cited papers.

If we do proceed with this change we might want to add a note somewhere in the doc clearly explaining the issue. It looks like the dMRI community have settled on the terminology order and all the papers I came across using CSD reconstruction report the maximum SH order when they talk about ℓmax (although it is true that the maximum SH order and degrees will be equal).

This is very confusing and unclear to me why there is a swap between the dMRI literature and wikipedia. I'll also tag @karanphil here who might have something to add.

It is indeed confusing. I remember being confused at first, coming from a physics background. However, when you read the literature on SH bases and CSD in dMRI (like tournier04, tournier07, descoteaux07 or descoteaux09), they all refer to l (or n in the case of Tournier/MRtrix) as the order of the SH function.

While Wikipedia follows the inverse terminology, other references such as Brilliant (https://brilliant.org/wiki/spherical-harmonics/) follow the current way of Dipy and the dMRI literature.

I think we need to be very careful if changing this.

skoudoro · 2024-02-28T21:12:02Z

Thank you for your feedback @CHrlS98. Indeed, Looking back #2206 reminds me that I have already this question as a comment.

@Garyfallidis and @arokem, Can you also give your opinion here? it is important. thank you!

maybe @RafaelNH could provide also a feedback

ebrahimebrahim · 2024-02-29T00:34:19Z

It looks like the dMRI community have settled on the terminology order and all the papers I came across using CSD reconstruction report the maximum SH order when they talk about $\ell_\text{max}$ (although it is true that the maximum SH order and degrees will be equal).

Hmm this is more confusing than I thought when I opened #2970

Sorry if I opened a can of worms that should have remained closed 😬

(In mrtrix "order" is used for $\ell$ but then it's not "degree" but "phase" that is used for $m$. So while using "order" for $\ell$ could match some dmri community conventions, I am questioning whether both $\ell$=order and $m$=degree is ever used together. IDK anymore...)

itellaetxe · 2024-03-01T10:56:08Z

We fixed the issue in master, Can you rebase this branch or merge master to this branch?

Hi @skoudoro, due to the ongoing discussion, I think I will leave it like this until we arrive to a conclusion. I will adapt the code to the final decision.

skoudoro · 2024-03-04T14:56:52Z

@arokem, we would like your feed back please when you have time

arokem · 2024-03-04T23:13:32Z

Hi! I think that we should use the terminology that is used in the dMRI papers. I would suggest putting in a Note that explains that this is a choice that we made, so readers of wikipedia and scipy should be careful when relying on these as resources. Does that makes sense?

ebrahimebrahim · 2024-03-04T23:52:09Z

Do dMRI papers swap degree and order or do they just use order for $\ell$ and then something different (like phase) for m?

CHrlS98 · 2024-03-05T03:18:50Z

In Maxime Descoteaux (@mdesco) PhD thesis, there is a chapter on spherical harmonics (section 5.2) where it is said that $\ell$ is the order and $m$, the degree (https://theses.hal.science/tel-00457458):

I am however having a hard time finding a peer-reviewed journal article referring to $m$ as the degree.

In Tournier et al, 2004 (https://doi.org/10.1016/j.neuroimage.2004.07.037), they refer to $\ell$ as the order, but $m$ is indeed called a phase factor:

Each spherical harmonic can be denoted using two numbers: its harmonic order n (n ≥ 0) and phase factor m (−n ≤ m ≤ n).

Actually, even in the DIPY paper from 2014 (https://doi.org/10.3389/fninf.2014.00008), $m$ is called the phase:

The SH of order l and phase m, Yml(θ, ϕ), arises from the angular solution to Laplace's equation in spherical coordinates and they form an orthonormal basis for complex functions defined on the unit sphere.

So, while there appears to be a consensus on $\ell$, I'm not really sure what we should call $m$ anymore...

arokem · 2024-03-05T04:41:17Z

@desco also used the term "phase factor" in [his seminal 2007 paper]:(https://onlinelibrary.wiley.com/doi/pdf/10.1002/mrm.21277)

I think part of the confusion stems from the fact that the words "order" and "degree" are used interchangeably to refer to the highest exponent in a polynomial function (see e.g., here). So, a nomenclature that uses "degree" and "phase factor" is potentially much less confusing. In fact, one could argue that if you ever use "order" that should refer to the variable referred to as $\ell$ in this writing of the equation.

In other words, potentially with a lot of explanation, I might suggest to go with order and phase_factor (I think that just phase may be confusing, because it's an integer and phases are usually thought of as continuous things.

skoudoro · 2024-03-05T05:02:41Z

In other words, potentially with a lot of explanation, I might suggest to go with order and phase_factor (I think that just phase may be confusing, because it's an integer and phases are usually thought of as continuous things.

+1 with this conclusion. Thanks all for the debate / clarification, I think we have a consensus.

@itellaetxe, can you go ahead with order (for $\ell$) and phase_factor for $m$ in the code. Concerning the documentation (rst file), you can refer to this PR for the discussion and add some additional notes.

I plan to release DIPY tomorrow or in 2 days so it will be great if you could update this PR on ASAP. Thank you in advance!

ps: it is ok if you can not make it.

skoudoro · 2024-03-05T05:06:19Z

PS: do not forget to fix the conflict and pep8 issues above. Thanks again !

…ew naming

itellaetxe · 2024-03-06T15:35:51Z

Thanks to everyone that took part in the discussion. Shortly:

I followed the indications given by @arokem and @skoudoro, regarding the new namings.
sh_degree_max is now sh_order_max.
Included the mentioned note in the theory rst file explaining our convention choice.
Fixed a couple of bad namings I overlooked the first time I opened the PR.
Passed tests locally.
Rebased my branch onto master.

I just saw that I made a typo with order in a couple of locations. Fixing and pushing now.

skoudoro · 2024-03-06T15:43:00Z

doc/theory/sh_basis.rst

+NOTE: 
+The definition of spherical harmonics that DIPY utilizes does not match the one
+in Wikipedia and scipy. Instead, DIPY follows the dMRI literature conventions,
+like in ``descoteaux07`` and ``tournier07``.
+The code in DIPY also follows the following convention:
+Let the SH be noted as $Y_{l}^m$. Then, $l$ is referred to as either order or
+l_value(s), and $m$ is referred to as either phase factor or m_value(s).
+This decisions were made as a result of the PR in https://github.com/dipy/dipy/pull/3086
+


karanphil · 2024-03-06T15:46:39Z

I am questioning the change from sh_order to sh_order_max. This would require at least deprecated versions for all functions with sh_order= as optional argument. For instance, reconst.shm.sf_to_sh will break now for any users using sh_order=.

I understand sh_order_max is more accurate, but is it really a necessary change?

I also found few places where l_value and m_value are arrays and are missing the s at the end, if we follow the summary of changes.

skoudoro · 2024-03-06T15:54:02Z

I understand sh_order_max is more accurate, but is it really a necessary change?

Let's try to be the most accurate as possible but you got a very good point @karanphil

@itellaetxe, everytime you change from sh_order to sh_order_max in a function argument, you need to add the decorator below:

@deprecated_params('sh_order', 'sh_order_max', since='1.9', until='2.0')

see an example:

dipy/dipy/align/imaffine.py

Lines 249 to 250 in 379062e

    
           @deprecated_params('interp', 'interpolation', since='1.13', until='1.15') 
        
           def _apply_transform(self, image, interpolation='linear',

CHrlS98 · 2024-03-06T16:05:35Z

doc/theory/sh_basis.rst

+The code in DIPY also follows the following convention:
+Let the SH be noted as $Y_{l}^m$. Then, $l$ is referred to as either order or
+l_value(s), and $m$ is referred to as either phase factor or m_value(s).
+This decisions were made as a result of the PR in https://github.com/dipy/dipy/pull/3086


"This decision was" or "These decisions were"?

Since this PR is cited there, I would also update the section "Main changes" (at the beginning of the PR) to represent the actual final changes. It would be less confusing if someone looks at it in the future.

Thank you both for the suggestions :)

@karanphil I just updated the beginning of the PR. It should reflect the current changes now.

@CHrlS98 thanks for the grammar check. I will leave it as "These decisions...". Pushed it.

Thanks @itellaetxe , just a small detail, I think the line "sh_degree_max is now sh_order_max" should be "sh_order is now sh_order_max", since sh_degree_max never really existed in Dipy.

…appy

… arg

itellaetxe · 2024-03-07T11:34:04Z

@itellaetxe, everytime you change from sh_order to sh_order_max in a function argument, you need to add the decorator below:

Done. It should be good to go now!

karanphil

I noted a couple of places where l/m_value should be l/m_values, and where the docstring is incomplete. Other than that, it looks good to me, great job!

dipy/data/__init__.py

dipy/reconst/csdeconv.py

dipy/reconst/forecast.py

dipy/reconst/mcsd.py

dipy/reconst/shm.py

codecov · 2024-03-08T14:39:22Z

Codecov Report

Attention: Patch coverage is 96.87500% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 81.80%. Comparing base (e050b8a) to head (e7c0fc2).
Report is 1 commits behind head on master.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3086      +/-   ##
==========================================
+ Coverage   81.76%   81.80%   +0.03%     
==========================================
  Files         150      150              
  Lines       21238    21284      +46     
  Branches     3412     3412              
==========================================
+ Hits        17365    17411      +46     
  Misses       3034     3034              
  Partials      839      839

Files	Coverage Δ
dipy/data/__init__.py	`79.64% <ø> (ø)`
dipy/direction/peaks.py	`84.15% <100.00%> (+0.32%)`	⬆️
dipy/nn/histo_resdnn.py	`86.51% <100.00%> (+0.30%)`	⬆️
dipy/reconst/mapmri.py	`91.86% <100.00%> (ø)`
dipy/workflows/reconst.py	`76.65% <100.00%> (+0.25%)`	⬆️
dipy/reconst/shm.py	`93.80% <98.88%> (+0.17%)`	⬆️
dipy/reconst/csdeconv.py	`87.42% <95.34%> (+0.19%)`	⬆️
dipy/reconst/forecast.py	`92.82% <93.33%> (+0.18%)`	⬆️
dipy/reconst/mcsd.py	`88.69% <94.87%> (+0.24%)`	⬆️

... and 4 files with indirect coverage changes

skoudoro · 2024-03-08T14:50:55Z

dipy/reconst/csdeconv.py

@@ -463,17 +466,17 @@ def estimate_response(gtab, evals, S0):
    return single_tensor(gtab, S0, evals, evecs, snr=None)


-def forward_sdt_deconv_mat(ratio, n, r2_term=False):
+def forward_sdt_deconv_mat(ratio, l_values, r2_term=False):


here, it is missing @deprecated_params('n', 'l_values', since='1.9', until='2.0')

skoudoro · 2024-03-08T14:53:38Z

dipy/reconst/shm.py

@@ -56,18 +57,18 @@ def bandit(f):
    return bandit


-def forward_sdeconv_mat(r_rh, n):
+def forward_sdeconv_mat(r_rh, l_values):


it is missing @deprecated_params('n', 'l_values', since='1.9', until='2.0')

skoudoro · 2024-03-08T14:54:48Z

dipy/reconst/shm.py



-def spherical_harmonics(m, n, theta, phi, use_scipy=True):
+def spherical_harmonics(m_values, l_values, theta, phi, use_scipy=True):


same, it is missing @deprecated_params(['m', 'n',], ['m_values', 'l_values'], since='1.9', until='2.0')

skoudoro · 2024-03-08T14:55:20Z

dipy/reconst/shm.py

    return val


 @deprecate_with_version('dipy.reconst.shm.real_sph_harm is deprecated, '
                        'Please use '
                        'dipy.reconst.shm.real_sh_descoteaux_from_index '
                        'instead', since='1.3', until='2.0')
-def real_sph_harm(m, n, theta, phi):
+def real_sph_harm(m_values, l_values, theta, phi):


skoudoro · 2024-03-08T14:55:35Z

dipy/reconst/shm.py



-def real_sh_tournier_from_index(m, n, theta, phi, legacy=True):
+def real_sh_tournier_from_index(m_values, l_values, theta, phi, legacy=True):


same here, see above

itellaetxe · 2024-03-08T14:55:51Z

@skoudoro, I completely overlooked the deprecator decorator in the m and n cases. I will fix it. Sorry!

skoudoro · 2024-03-08T14:55:52Z

dipy/reconst/shm.py

    else:
        warn(tournier07_legacy_msg, category=PendingDeprecationWarning)

    return real_sh


-def real_sh_descoteaux_from_index(m, n, theta, phi, legacy=True):
+def real_sh_descoteaux_from_index(m_values, l_values, theta, phi, legacy=True):


same here, see above

skoudoro

Thank you for this great work @itellaetxe.

This PR is almost ready to go. See my additional comments.

is there any additional comments @CHrlS98 ? @karanphil ? @arokem ? @ebrahimebrahim ?

skoudoro · 2024-03-08T15:03:39Z

If not, I might edit this PR, let the CI's run and merge it today to be part of the release.

skoudoro · 2024-03-08T20:00:22Z

Ok, all seems ok, merging!

Thank you @itellaetxe for this work

Thank you @arokem @CHrlS98 @karanphil @ebrahimebrahim for the review/debate

itellaetxe · 2024-03-11T18:15:34Z

Thank you @skoudoro for the help with those last decorators!
Also thank you for the patience and the good advice to everyone that participated in the discussion. I will try to do it better next time :)

itellaetxe changed the title ~~Fix spherical harm terminology swap~~ Fix spherical harmonic terminology swap Feb 28, 2024

itellaetxe changed the title ~~Fix spherical harmonic terminology swap~~ [RF] Fix spherical harmonic terminology swap Feb 28, 2024

itellaetxe added 5 commits March 6, 2024 16:16

[RF] Standardized spherical harmonic order and degree notation in code

cb0ad6f

[RF] Updated examples according to new naming

a67c256

[RF] Updated benchmarks, doc/interfaces and doc/theory according to n…

dacdf41

…ew naming

[PL] Shortened lines below 80 char limit

ad55423

[RF] Changed order/degree to order/phase factor

8d07316

itellaetxe force-pushed the fix_sph_harm_terminology_swap branch from 24fb046 to 8d07316 Compare March 6, 2024 15:28

[STYLE] Fixed -> typos

1f5e5a8

skoudoro reviewed Mar 6, 2024

View reviewed changes

CHrlS98 reviewed Mar 6, 2024

View reviewed changes

[TEST] Fixed reconst workflow tests with new 'sh_order_max' argument

cdfb67d

itellaetxe added 2 commits March 7, 2024 10:23

[DOC][PL] Corrected grammar in sh_basis footnote, make spellchecker h…

a183329

…appy

[BW] Added deprecators of 'sh_order' in favor of 'sh_order_max' input…

f367efe

… arg

karanphil reviewed Mar 7, 2024

View reviewed changes

itellaetxe added 2 commits March 8, 2024 12:51

[RF] Fixed errata of m/l values naming when they were arrays and not int

8a3d522

[TEST] Change HistoResDNN decorator order to make test run

e7c0fc2

skoudoro reviewed Mar 8, 2024

View reviewed changes

skoudoro added 5 commits March 8, 2024 13:37

add decorators for m_values, l_values

c06726b

Merge branch 'master' into fix_sph_harm_terminology_swap

aecb8df

[RF] add forgotten import

2fe82a3

using github interface to edit is a bad idea...

e1db757

Update workflows.reconst.py

d75eedd

skoudoro merged commit 4927afe into dipy:master Mar 8, 2024
26 of 28 checks passed

skoudoro mentioned this pull request Mar 8, 2024

spherical harmonic degree/order terminology swapped #2970

Closed

arokem mentioned this pull request May 17, 2024

New appendix page: spherical harmonics bids-standard/bids-bep016#104

Open



		def spherical_harmonics(m, n, theta, phi, use_scipy=True):
		def spherical_harmonics(m_values, l_values, theta, phi, use_scipy=True):



		def real_sh_tournier_from_index(m, n, theta, phi, legacy=True):
		def real_sh_tournier_from_index(m_values, l_values, theta, phi, legacy=True):

[RF] Fix spherical harmonic terminology swap #3086

[RF] Fix spherical harmonic terminology swap #3086

Conversation

itellaetxe commented Feb 28, 2024 • edited

Summary

Main changes

pep8speaks commented Feb 28, 2024 • edited

Comment last updated at 2024-03-08 19:24:55 UTC

itellaetxe commented Feb 28, 2024

skoudoro commented Feb 28, 2024

CHrlS98 commented Feb 28, 2024

karanphil commented Feb 28, 2024

skoudoro commented Feb 28, 2024

ebrahimebrahim commented Feb 29, 2024 • edited

itellaetxe commented Mar 1, 2024

skoudoro commented Mar 4, 2024

arokem commented Mar 4, 2024

ebrahimebrahim commented Mar 4, 2024

CHrlS98 commented Mar 5, 2024

arokem commented Mar 5, 2024

skoudoro commented Mar 5, 2024 • edited

skoudoro commented Mar 5, 2024

itellaetxe commented Mar 6, 2024

Choose a reason for hiding this comment

karanphil commented Mar 6, 2024 • edited

skoudoro commented Mar 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itellaetxe Mar 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itellaetxe commented Mar 7, 2024 • edited

karanphil left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 8, 2024 • edited

Codecov Report

skoudoro Mar 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itellaetxe commented Mar 8, 2024

Choose a reason for hiding this comment

skoudoro left a comment

Choose a reason for hiding this comment

skoudoro commented Mar 8, 2024

skoudoro commented Mar 8, 2024

itellaetxe commented Mar 11, 2024

itellaetxe commented Feb 28, 2024 •

edited

pep8speaks commented Feb 28, 2024 •

edited

ebrahimebrahim commented Feb 29, 2024 •

edited

skoudoro commented Mar 5, 2024 •

edited

karanphil commented Mar 6, 2024 •

edited

skoudoro commented Mar 6, 2024 •

edited

itellaetxe Mar 7, 2024 •

edited

itellaetxe commented Mar 7, 2024 •

edited

codecov bot commented Mar 8, 2024 •

edited

skoudoro Mar 8, 2024 •

edited