forward-mode jacobian aborts for mappings R^n -> R^m with m != n - #3386
Open
SteveBronder wants to merge 2 commits into
Open
forward-mode jacobian aborts for mappings R^n -> R^m with m != n#3386SteveBronder wants to merge 2 commits into
SteveBronder wants to merge 2 commits into
Conversation
Regression tests for #3385. Both fail against develop, aborting inside jacobian() before reaching any assertion: Assertion failed: (rows == this->rows() && cols == this->cols() && "DenseBase::resize() does not actually allow to resize.") The fwd jacobian sizes J as n x n and fx as n from the input before evaluating f, so J.col(0) = fx_fvar.d() assigns an m-vector into an n-row column whenever m != n. Existing coverage in test/unit/math/mix/functor/autodiff_test.cpp only uses an R^2 -> R^2 functor, where m == n masks the mis-sizing entirely. These cover both directions: R^2 -> R^3 (more outputs than inputs, the case forward mode is actually cheaper for) and R^3 -> R^2, each checked against a hand-computed Jacobian. The tests live in fwd/ and include only stan/math/fwd.hpp, which is also the realistic reproduction path: with rev in scope the rev overload wins partial ordering and handles these functors correctly.
The fwd jacobian sized both outputs from the domain before f was ever
applied, so the range dimension m was simply assumed equal to n:
J.resize(x_fvar.size(), x.size());
fx.resize(x_fvar.size());
...
Matrix<fvar<T>, Dynamic, 1> fx_fvar = f(x_fvar);
J.col(0) = fx_fvar.d();
For m != n that assigns an m-vector into an n-row column. Move the
J.resize below the call to f and take its rows from fx_fvar.size(),
which mirrors what the rev implementation already does.
The fx.resize is dropped rather than moved: fx = fx_fvar.val() is a
full-matrix assignment that resizes on its own, which is why fx.size()
came out correct even in the aborting runs. Moving it would only have
restated that.
Failure modes before the fix, both directions, with a functor that is
not square:
assertions on (math's own build never defines NDEBUG) - abort.
-DNDEBUG, m > n - Eigen's assignment loop bounds on the destination
block, so the copy truncates: J stays n x n and holds a partial
Jacobian. Silent wrong answer, no out-of-bounds access.
-DNDEBUG, m < n - the same destination bound reads past the source
temporary. ASan reports a heap-buffer-overflow READ at jacobian.hpp:25,
8 bytes past fx_fvar.
Note that this narrows the issue's description: there is no write past
J's storage in either direction, so no heap corruption.
No signature change, so overload resolution is untouched and the eight
in-tree callers (all in rev/functor, all resolving to the rev overload,
and all square) are unaffected. Verified with ASan clean in both
directions, agreement with the rev overload on the same functor, the
higher-order T = fvar<double> instantiation, the fwd/functor suite,
mix/functor/autodiff_test, and rev/functor/algebra_solver_fp_test.
Closes #3385
WardBrian
approved these changes
Sep 3, 2026
WardBrian
left a comment
WardBrian
left a comment
Member
There was a problem hiding this comment.
Fix seems simple enough! Hilarious that the LLM thinks it's worth writing ~400 words worth of commit messages for.
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
2 participants
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.
Summary
Fixes #3385
Tests
red/green test for showing the issue and then resolving it.
Checklist
Copyright holder: Steve Bronder
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested