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

Confusing documentation for scipy.special.sph_harm #6657

Closed
EmperorDali opened this issue Oct 6, 2016 · 9 comments
Closed

Confusing documentation for scipy.special.sph_harm #6657

EmperorDali opened this issue Oct 6, 2016 · 9 comments
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org good first issue Good topic for first contributor pull requests, with a relatively straightforward solution scipy.special
Milestone

Comments

@EmperorDali
Copy link

EmperorDali commented Oct 6, 2016

The documentation for scipy.special.sph_harm states that this function computes

Y_n^m(\theta,\phi) = \sqrt{\frac{2n+1}{4\pi}\frac{(n-m)!}{(n+m)!}} e^{im\theta}P_n^m(\cos(\phi))

I am not an expert in spherical harmonics, but to my eyes this notation imples that scipy is computing spherical harmonics without the Condon-Shortley phase factor.

However, scipy is actually computing the spherical harmonics with CS phase:

sph_harm(1,1,0,np.pi/2)
(-0.3454941494713355+0j)

compare this to

Y_1^1(\theta,\phi) = \sqrt{\frac{3}{4\pi}}e^{i\theta}(-1)(1-\cos(\phi))^2

which is a spherical harmonic with CS phase, see table here

Presumably this is because scipy includes the CS phase factor with the associated Legendre function scipy.special.lpmv. I believe this is another convention that is used, however the documentation for scipy.special.lpmv doesn't state what exactly is being computed.

We should 1) update the documentation for lpmv to indicate that the function computes the associated Legendre polynomials with CS phase 2) add a note to the documentation for sph_harm indicating that the function returns spherical harmonics with CS phase.

@larsoner
Copy link
Member

larsoner commented Oct 6, 2016

Sounds good to me. We should also 3) add tests for this to be sure, via manual evaluation of the equation and/or cross-checking with other packages. Do you have time to make a PR?

@person142
Copy link
Member

Hey @EmperorDali note that the wiki page defines

Y_1^1(theta, phi) = stuff*exp(i*phi)*sin(theta)

whereas SciPy defines

Y_1^1(theta, phi) = stuff*exp(i*theta)*sin(phi).

If I flip the arguments in your example I get

>>> sph_harm(1, 1, np.pi/2, 0)
(-0+0j)

which matches the wiki expression. So I think everything defined correctly in the documentation.

@larsoner
Copy link
Member

larsoner commented Oct 6, 2016

Oh great, the multiple definitions of spherical harmonics strike again...

@ev-br
Copy link
Member

ev-br commented Oct 6, 2016

Given the amount of confusion this generates over the years, it'd probably be good to add an explicit form of the low-order harmonics (m=1, l=-1, 0, 1) and mention the Condon-Schockley phase.

@ev-br ev-br added Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.special good first issue Good topic for first contributor pull requests, with a relatively straightforward solution labels Oct 6, 2016
@person142
Copy link
Member

And while someone was working on that, they could also add a full definition of lpmv and mention the Condon-Shortley phase (or lack thereof) there too.

@EmperorDali
Copy link
Author

@person142 I don't agree with what you wrote. Note that when I copied Y_1^1 from the table to my first post I interchanged \theta and \phi, to match scipy's convention.

If you look at my post, you'll see I did stuff*exp(i*theta)*sin(phi), both in scipy code and in direct computation by Y_1^1

@person142
Copy link
Member

Ah, I worked directly from the wiki page and missed that. Apologies.

The function lpmv is in specfun, so I cracked open Zhang and Jin who indeed define

P_v^m(x) = (-1)^m (1 - x^2)^{m/2} (d^m/dx^m) P_v(x).

@person142
Copy link
Member

person142 commented Oct 7, 2016

The refguide for the Legendre functions could also use some improvements:

https://scipy.github.io/devdocs/special.html#legendre-functions

In particular it's difficult to figure out the differences between the functions at a glance. Also, given the names of the functions (consider lpmv and lpmn, neither of which are very descriptive) I'd say that adding some aliases would be a good idea.

Edit: aliases are a bit OT for this issue though; just something to think about.

@larsoner
Copy link
Member

larsoner commented Oct 8, 2016

Yes it could be cleaned up a bit. I'll see if I can find some time to do it.

person142 added a commit to person142/scipy that referenced this issue Nov 1, 2016
Also add clearer one-sentence summaries for the Legendre
functions. Closes scipygh-6657.
@pv pv closed this as completed in #6746 Nov 1, 2016
@pv pv added this to the 0.19.0 milestone Nov 1, 2016
Linkid pushed a commit to Linkid/scipy that referenced this issue Feb 9, 2017
Also add clearer one-sentence summaries for the Legendre
functions. Closes scipygh-6657.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org good first issue Good topic for first contributor pull requests, with a relatively straightforward solution scipy.special
Projects
None yet
Development

No branches or pull requests

5 participants