Fix wiener_lpdf derivative with respect to w#3324
Conversation
Correct the reverse-mode derivative of wiener_lpdf with respect to the relative starting point parameter w. The previous implementation disagreed with finite differences of Stan Math's own scalar value function in the 5-parameter overload and in the full overload. The fix evaluates the w derivative in the same coordinate and branch convention as the density calculation. Add expect_ad regression tests for the failing 5-parameter case, an sv=0 control, the full sw>0, st0=0 case, and the existing full-Wiener rows. Update the stale full-Wiener row 4 w-gradient reference to the corrected finite-difference/fixed-AD value.
There was a problem hiding this comment.
Pull request overview
Fixes the reverse-mode derivative of wiener_lpdf with respect to the relative starting point parameter w by aligning the w-gradient’s series representation (coordinates + branch choice) with the density calculation, and updates tests/reference gradients accordingly.
Changes:
- Reworked
internal::wiener5_grad_wto compute thewderivative using the same branch convention as the density (includingq = 1 - win the large-time branch). - Updated the stored full-Wiener reference gradient for
w(row 4) to the corrected value. - Added a new mix-mode AD regression test suite targeting the previously failing cases and a reverse-only sweep over existing full-Wiener rows.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
stan/math/prim/prob/wiener5_lpdf.hpp |
Updates the w-gradient implementation to match the density’s branch/coordinate conventions. |
test/unit/math/prim/prob/wiener_full_lpdf_test.cpp |
Updates the expected w gradient reference value for the corrected row. |
test/unit/math/mix/prob/wiener_lpdf_ad_test.cpp |
Adds AD regression tests to validate the corrected w derivative in mix-mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const auto y_asq = y / square(a); | ||
| const auto q = 1.0 - w; | ||
| const auto sv_sqr = square(sv); |
|
@Franzi2114 can you take a look at this? |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
I will have a look at it, but it may take some time. |
|
If you'd like to discuss / have questions, feel free to reach out, I am available on the Stan slack and martonarovarga at gmail dot com as well. Thanks a lot! |
|
And I'm not reviewing the C++ or Stan code or language parser, but I'm assuming this PR was opened by a competent developer, and the Stan code was tested on simulated data, in R, python, whatever. But should we be updating posteriorDB with models when we add new PDF's, etc, since we're assuming they've already implemented/tested the Stan model on simulated data thoroughly? Possibly automated between different repos, IDK |
|
Idt we can automate that, but people are always welcome to contribute models to posterior DB if they want to |
Summary
Fixes #3322.
This PR fixed the reverse-mod derivative of
wiener_lpdfwith respect to the relative starting point parameterw.The previous implementation disagreed with finite differences of Stan Math’s own scalar value function in both the 5-parameter overload and the full overload. In the failing full-Wiener row, the old reverse-mode adjoint was
After this PR, the reverse-mode adjoint agrees with the finite-difference value:
The implementation change is local to
stan/math/prim/prob/wiener5_lpdf.hpp. Thewderivative is now evaluated using the same upper-bound coordinate and branch convention as the density calculation. In particular, the large-time branch uses the coordinateq = 1 - w, consistent with the density representation, and applies the chain rule with respect tow.This PR also updates the stale full-Wiener row 4
wreference value inwiener_full_lpdf_test.cppfrom the previous adjoint/reference value to the finite-difference/fixed-AD value.Tests
Added
test/unit/math/mix/prob/wiener_lpdf_ad_test.cpp, usingstan::test::expect_ad.The new tests cover:
sv = 0control case,sw > 0, st0 == 0,wadjoint.The existing-row sweep uses reverse-only
expect_ad<true>because one row triggers an unrelated higher-ordergrad_hessiantolerance issue, while this PR fixes the first derivative with respect tow.Locally run:
and all
wiener*matches intest/.Side Effects
This changes the reverse-mode adjoint of
wiener_lpdfwith respect towin regimes where the previous implementation had the wrong coordinate/sign convention.This may change gradients, optimization paths, and posterior geometry for models using
wiener_lpdfwith autodiff onw. The log density values are unchanged.Release notes
Fix incorrect reverse-mode derivative of
wiener_lpdfwith respect to the relative starting point parameterw.Checklist
Targeted tests pass locally. I have not run the full
./runTests.py test/unit,make test-headers,make test-math-dependencies,make doxygen, ormake cpplintlocally.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:
)
)