fix(prediction/sph_harm): handle scipy 1.15 sph_harm_y API change - #4
Open
k-yoshimi wants to merge 2 commits into
Open
fix(prediction/sph_harm): handle scipy 1.15 sph_harm_y API change#4k-yoshimi wants to merge 2 commits into
k-yoshimi wants to merge 2 commits into
Conversation
SciPy 1.15 replaced scipy.special.sph_harm with sph_harm_y, inverting *both* the (m, n) -> (n, m) argument order and the meaning of theta/phi: sph_harm (deprecated): sph_harm(m, n, theta_az, phi_pol) sph_harm_y (>= 1.15): sph_harm_y(n, m, theta_pol, phi_az) The previous try/except ImportError only swapped the binding name, leaving every call site as _sph_harm(m, l, phi, theta) -- correct under the legacy API, wrong under sph_harm_y. As a result, on scipy >= 1.15 every l>0 channel (9 of 16 spherical-harmonic outputs in our l_max=3 setup) silently returned zero. The discrete normalization integral collapsed from ~1.0 to ~0.27 and <|psi|^2> dropped from ~1e-3 to ~2.7e-4 on the 32^3 grid with L=10, distorting all downstream training data and the reported MSE band. This commit wraps both APIs in a single helper _Ylm(l, m, phi_az, theta_pol) that always evaluates Y_l^m(theta_pol, phi_az) in the standard physics convention regardless of which scipy version is installed; the three call sites in spherical_harmonics() are updated accordingly. No behavior change on scipy < 1.15.
The existing tests for spherical_harmonics() check only output shape
and dtype, so they pass even when every l > 0 channel returns zero --
the exact symptom of the SciPy 1.15 sph_harm_y argument-order
regression that the previous commit fixes.
Add a TestSphericalHarmonicsValues class with three layers of
defense against a recurrence:
* Closed-form reference values at fixed (theta, phi) for
Y_{0,0}, Y_{1,0}, Y_{1,+/-1}, Y_{2,0}, Y_{2,+/-2}, covering both
the m > 0 (Re), m < 0 (Im), and m = 0 branches of the wrapper.
* test_no_silent_zero_channel(): for every (l, m) with 0 <= l <= 3,
evaluates the harmonic on 128 randomized angles and asserts that
at least one value exceeds 1e-6 in magnitude. This is the direct
detector of the original silent-zero regression.
* test_sum_rule_per_l(): on randomized angles, verifies the
Unsoeld-style sum rule
sum_{m=-l..l} Y_{l,m}(theta, phi)^2 = (2 l + 1) / (4 pi)
which is independent of the (m, n) argument-order convention,
so it is a strong cross-check on any future scipy backend swap.
Also tightens the comment in test_import_fallback_mechanism to refer
to the renamed _scipy_sph backend symbol introduced in the previous
commit.
There was a problem hiding this comment.
Pull request overview
Updates the spherical-harmonics implementation to be correct across SciPy versions by normalizing the SciPy 1.15 sph_harm_y API change (argument order and theta/phi meaning) behind a single internal helper.
Changes:
- Introduce
_Ylm(l, m, phi_az, theta_pol)wrapper that calls eitherscipy.special.sph_harm_y(SciPy ≥ 1.15) or legacyscipy.special.sph_harm(older SciPy) with the correct parameter mapping. - Update
spherical_harmonics()to use_Ylmso all call sites get consistent behavior regardless of SciPy version.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
13
to
+22
| try: | ||
| from scipy.special import sph_harm_y as _sph_harm | ||
| from scipy.special import sph_harm_y as _scipy_sph | ||
| def _Ylm(l, m, phi_az, theta_pol): | ||
| # sph_harm_y(n=l, m=m, theta=theta_pol, phi=phi_az) | ||
| return _scipy_sph(l, m, theta_pol, phi_az) | ||
| except ImportError: | ||
| # Fallback for older scipy versions | ||
| from scipy.special import sph_harm as _sph_harm | ||
| from scipy.special import sph_harm as _scipy_sph | ||
| def _Ylm(l, m, phi_az, theta_pol): | ||
| # sph_harm(m=m, n=l, theta=phi_az, phi=theta_pol) | ||
| return _scipy_sph(m, l, phi_az, theta_pol) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SciPy 1.15 replaced scipy.special.sph_harm with sph_harm_y, inverting both the (m, n) -> (n, m) argument order and the meaning of theta/phi:
sph_harm (deprecated): sph_harm(m, n, theta_az, phi_pol)
sph_harm_y (>= 1.15): sph_harm_y(n, m, theta_pol, phi_az)
The previous try/except ImportError only swapped the binding name, leaving every call site as _sph_harm(m, l, phi, theta) -- correct under the legacy API, wrong under sph_harm_y. As a result, on scipy >= 1.15 every l>0 channel (9 of 16 spherical-harmonic outputs in our l_max=3 setup) silently returned zero. The discrete normalization integral collapsed from ~1.0 to ~0.27 and <|psi|^2> dropped from ~1e-3 to ~2.7e-4 on the 32^3 grid with L=10, distorting all downstream training data and the reported MSE band.
This commit wraps both APIs in a single helper _Ylm(l, m, phi_az, theta_pol) that always evaluates Y_l^m(theta_pol, phi_az) in the standard physics convention regardless of which scipy version is installed; the three call sites in spherical_harmonics() are updated accordingly. No behavior change on scipy < 1.15.