Fix SphericalBesselFunction(smooth=True): float32 basis constants, dtype-dependent output, dead lru_cache - #826
Merged
shyuep merged 1 commit intoAug 25, 2026
Conversation
toohardtogetname
marked this pull request as ready for review
August 25, 2026 05:13
…he symbolic-function cache effective SphericalBesselFunction(smooth=True) fed its cutoff BUFFER into the symbolic builder, so cutoff**1.5 was evaluated in the ambient default dtype: under float32 the basis prefactor is rounded once and every generated coefficient is scaled by 1 + 2.6e-8. The basis therefore disagreed with matgl's own spherical_bessel_smooth() (which the LAMMPS export path substitutes as 'mathematically identical') and silently changed with torch.set_default_dtype. Passing float(self.cutoff) fixes both, and float32 output is bit-identical before/after (the correction is below one float32 ULP; nothing needs retraining). The same change makes the module-level lru_cache on _get_lambda_func actually hit (a tensor argument hashes by identity), saving ~220 ms of sympy.simplify per constructed module. The two method-level lru_cache decorators are removed: keyed on self they can never hit, while pinning up to 128 dropped modules against garbage collection. Three regression tests: closed-form agreement, default-dtype independence, cache reuse. All three fail on 4.0.3 and pass with this change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
toohardtogetname
force-pushed
the
fix-smooth-sbf-fp32-basis
branch
from
August 25, 2026 05:29
d59bed1 to
f882fb0
Compare
Contributor
|
Thanks! |
toohardtogetname
pushed a commit
to toohardtogetname/matgl
that referenced
this pull request
Aug 25, 2026
…; adopt materialyzeai#825's captured-local lambda style in item-5 hunks Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Affects every model constructed with
use_smooth=True(M3GNet, TensorNet andQET all expose the flag; the TensorNet MatPES PES checkpoints ship with it
enabled). Reproduced on matgl 4.0.3 and on
mainas of 2026-08-22, torch 2.9.1,sympy 1.14.0.
float32 output does not change at all (0 of 200,001 sampled points move, 0.00
ULP), so this is not a "your results were wrong" report and nothing needs
retraining. What it costs today is reproducibility, float64 accuracy, and about
220 ms of
sympy.simplifyper model constructed.Symptom 1: matgl's two implementations of the same basis disagree
matgl contains two implementations of the smooth spherical-Bessel basis:
SphericalBesselFunction(smooth=True)(sympy, lambdified) and the hand-writtenspherical_bessel_smooth(). Evaluated on the same points in float64 they differ:spherical_bessel_smoothis the correct one: it spells its constants outexactly (
sqrt2 = 1.4142135623730951,pi_local = 3.141592653589793).This matters beyond a numerical curiosity, because matgl already treats the two
as interchangeable.
matgl.ext._lammps._SmoothSBFExpansionsubstitutesspherical_bessel_smoothforBondExpansion(rbf_type='SphericalBessel', smooth=True)when exporting a model for LAMMPS, on the stated grounds that it"is mathematically identical". It should be — and after this fix it is. Today a
LAMMPS-exported model and the Python model evaluate slightly different bases.
Symptom 2: the basis depends on the ambient default dtype
The same model definition and the same checkpoint therefore produce different
radial features depending on whether something earlier in the process called
torch.set_default_dtype. Nothing warns you.The exact value is
2*sqrt(2)/5 = 0.565685424949238; the float32 path is high by2.576e-08relative, and every coefficient of everynis off by that samefactor.
Cause
__init__stores the cutoff as a buffer, so its dtype follows the global default:and the smooth branch passes that tensor into the symbolic builder unchanged:
_get_lambda_funcevaluatescutoff**1.5while assembling the basis prefactor.With a float32 buffer that is a float32 op, so the prefactor is rounded once and
the rounding scales every generated coefficient.
The non-smooth branch of the same
__init__already avoids this — it writesfactor = sqrt(2.0 / float(cutoff) ** 3).Symptom 3: the symbolic functions are rebuilt for every module
_get_lambda_funcislru_cached, but a tensor argument hashes by identity, sothe cache never hits and each construction re-runs
sympy.simplify:_get_lambda_funcon a cache miss_get_lambda_funcon a cache hitSeparately, the
@lru_cacheon_calculate_smooth_symbolic_funcskeys onself. The method is called exactly once per instance, so that cache can neverhit either — while holding a strong reference to every module constructed, up to
128 of them. Eight modules dropped and garbage-collected leave eight alive.
_calculate_symbolic_funcs(the non-smooth branch) carries the same decoratorand the same problem.
Fix
See
fix.diff: passfloat(self.cutoff), and drop the two method-levellru_cachedecorators that cannot hit.Backward compatibility: float32 output is bit-identical
The correction is smaller than a float32 ULP everywhere, so no existing float32
result changes and no retraining is needed. Over 200,001 points on
r in [0.3, 5.0]:Agreement with the closed form
sqrt(2)(2 sin(pi r/5) + sin(2 pi r/5))/(5 r)improves from2.576e-08to1.268e-15, and the two implementations above come into agreement.Who this actually affects
reimplementation (a LAMMPS pair style, a rewritten kernel, another framework)
against matgl meets a
2.6e-08floor that is not in their own code. This ishow the bug was found.
torch.set_default_dtype(torch.float64)— common forphonons and finite-difference work — silently get a different basis than
workflows that do not.
test suites, all pay ~220 ms of avoidable sympy work per model.
Tests
test_smooth_sbf_basis.pyadds three regression tests: closed-form agreement,default-dtype independence, and cache reuse. All three fail on 4.0.3 and pass
with the fix.
🤖 Generated with Claude Code