Apply quadrature weights out of place so autograd survives GaussLegendre - #262
Open
gomezzz wants to merge 2 commits into
Open
Apply quadrature weights out of place so autograd survives GaussLegendre#262gomezzz wants to merge 2 commits into
gomezzz wants to merge 2 commits into
Conversation
evaluate_integrand ended with `result *= weights`, mutating the tensor the user's integrand had just returned. That broke PyTorch autograd through GaussLegendre for any integrand whose last operation reads its own output in the backward pass (exp, sqrt, tanh, sigmoid, div, pow), raising "one of the variables needed for gradient computation has been modified by an inplace operation". It failed the same way for gradients with respect to the integration domain, on CPU and GPU. Only the Gaussian family passes weights through this path, so Newton-Cotes and Monte Carlo were unaffected, as was JAX. Present since 0.4.0. The multiplication is unchanged; only its destination is. As a side effect the weights are no longer downcast to the integrand's dtype, so a float32 integrand under float64 precision now keeps the weights' precision. gradient_test.py listed six integrators but only five point counts, so zip silently dropped GaussLegendre -- the only integrator that reaches this code -- from every gradient test. The lists are now length-checked and GaussLegendre gets more 1D points, since the V-shaped test integrands have a kink and Gauss-Legendre is only O(N^-2) there. A smooth exponential integrand is added because every existing test function has a backward pass that does not read its own output, and so could never have caught this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YP9oDLmw636HhPSAD6Nv3p
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
3 tasks
…ights Review follow-up. Applying the weights out of place stopped the silent downcast but introduced a silent upcast, which the fail-hard rule objects to just as much. A dtype mismatch is now warned about, matching the backend-mismatch warning ten lines above it. The check compares mantissa width, so a complex128 integrand against float64 weights stays quiet -- it loses no precision -- while complex64 or float32 against float64 does not. The no-mutation and dtype guarantees move from an inline comment into the evaluate_integrand docstring: it is public API, and the tutorial shows users calling it directly when writing custom integrators. In gradient_test.py the three parallel lists become one list of tuples, so they cannot drift out of sync again rather than being caught by an assert. The exponential case now skips MonteCarlo and VEGAS, which never reach the weights branch and were already covered by the four preceding integrands, and its bound tightens from 5e-2 to 1e-4 -- the former was set by Monte Carlo and would have passed a several-percent error in weight application on the deterministic rules. The changelog entry drops the test-suite detail, which users do not ship, and splits the dtype change into Changed where it belongs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YP9oDLmw636HhPSAD6Nv3p
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The precision table compares storage width rather than significand precision: bfloat16 and float16 are both mapped to 16 even though bfloat16 is less precise. A bfloat16 integrand with float16 weights therefore skips this warning. Please rank by actual mantissa precision and add that mixed-dtype case to the tests.
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.
BaseIntegrator.evaluate_integrandended withresult *= weights— an in-placemultiply on the tensor the user's integrand had just returned. torchquad does not
own that tensor.
reads its own output in the backward pass —
exp,sqrt,tanh,sigmoid,div,pow— raisesRuntimeError: one of the variables needed for gradient computation has been modified by an inplace operation ... is at version 1; expected version 0. Same failure for gradients with respect to the integrationdomain, on CPU and GPU. Only the Gaussian family passes
weightshere(
GridIntegrator._weightsreturnsNone), so Newton–Cotes and Monte Carlo wereunaffected, as was JAX. Present since 0.4.0 (Gaussian quadrature #141).
gradient_test.pylisted six integrators but onlyfive point counts, so
zipsilently droppedGaussLegendre— the onlyintegrator that reaches this code — from every gradient test. And every existing
test integrand (
2|x|, polynomials) has a backward pass that does not read itsown output, so even with GaussLegendre restored they would all still pass.
Found while validating the 0.6.0 release; not a 0.6.0 regression. It fails
loudly rather than returning wrong numbers, so no published result is at risk.
Related to #258.
Numerical behaviour
The multiplication is identical; only its destination changed. No tolerance was
loosened, and the full suite passes unchanged on all four backends.
One deliberate behaviour change: an in-place multiply takes its left operand's
dtype, so an integrand returning
float32underfloat64precision silentlydiscarded the weights' precision. Out-of-place promotes instead, which is what
"no silent precision downgrades" requires.
tests/base_integrator_test.py::test_weight_dtype_*pins this.
GaussLegendregets 499 rather than 149 points in the 1-D gradient tests: theV-shaped integrands there have a kink, and Gauss–Legendre is only
O(N^-2)on|x|while being spectrally accurate on the smooth exponential at any of theseN. RaisingNrather than loosening a tolerance.Test plan
tests/base_integrator_test.py— 10 tests asserting the integrand'sreturned tensor is unchanged after
evaluate_integrand, for scalar andmulti-value integrands, on numpy/torch/jax/tensorflow, plus the dtype guard.
Verified these fail without the fix (numpy and torch; jax and tensorflow
pass trivially since their tensors are immutable).
gradient_test.py—GaussLegendrerestored to the matrix with alength assertion, and a new exponential integrand whose exact gradient is
1.0. Verified this fails without the fix with the preciseExpBackward0error, and passes with it, on torch/jax/tensorflow.ruff checkclean,ruff format --checkclean (64 files),pydoclint torchquad/no violations,vulture --min-confidence 100clean.d/da ∫₀² e^{-a x²} dxata = 0.7gives-0.6561422331326556versus a fine-grid reference of-0.6561422331326168.What changed after review
silent downcast but replaced it with a silent promotion, which the fail-hard
rule objects to equally. A dtype mismatch now warns, matching the
backend-mismatch warning ten lines above it. The check compares mantissa width,
so a
complex128integrand againstfloat64weights stays quiet — it loses noprecision — while
float32orcomplex64againstfloat64does not. Verifiedagainst the full suite: the only warnings emitted are the four pre-existing
Boole/Simpson
N-adjustment ones.evaluate_integrandis public APIand the tutorial shows users calling it directly; a contract that lives only in
an inline comment is one refactor from being reintroduced as
*=.gradient_test.pybecame one list of tuples, so theycannot drift out of sync again rather than being caught by an assert after the
fact.
weights branch and were already covered by the four preceding integrands, and
its bound tightened from
5e-2to1e-4. The old bound was set by Monte Carloand would have passed a several-percent error in weight application on the
deterministic rules;
1e-4is limited by Trapezoid at1.7e-5, while Boole andGaussLegendre reach
1e-15.integrand stays quiet.
Not addressed here
Gaussian._weights(self, N, dim, backend, requires_grad=False)never receivesrequires_gradfrom any of its four callers, soweights.requires_grad = requires_gradis a permanent no-op. Confirmed, but unrelated to this fix andbetter as its own change —
_rootsdoes use the parameter, so it is not ablanket removal.