Fix missing signs factor in bernoulli_logit_lpmf partials above the cutoff - #3370
Closed
sims1253 wants to merge 2 commits into
Closed
Fix missing signs factor in bernoulli_logit_lpmf partials above the cutoff#3370sims1253 wants to merge 2 commits into
sims1253 wants to merge 2 commits into
Conversation
…cutoff In the (ntheta > cutoff) branch the value is -exp(-ntheta), so the derivative with respect to theta is signs * exp(-ntheta) with signs = 2n - 1. The partials expression returned -exp_m_ntheta instead, which is only correct for n = 0; for observations with n = 1 and theta > 20 (and, symmetrically, n = 0 with theta < -20) the gradient of every such element has the wrong sign. The per-element error is 2 * exp(-ntheta) (<= 4e-9 just above the cutoff), which is presumably why it went unnoticed. The same branch already applies signs correctly in the middle and lower branches; this makes the upper branch consistent. Adds a regression test checking the autodiff gradient against both the analytic value and central finite differences of the double implementation, staying inside the same branch; it fails on the previous code.
…espace/line_length)
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.
bernoulli_logit_lpmfcomputes, per observation, withsigns = 2n − 1andntheta = signs * theta:exp_m_ntheta = exp(-ntheta); logp = (ntheta > cutoff) .select(-exp_m_ntheta, (ntheta < -cutoff).select(ntheta, -log1p(exp_m_ntheta))); edge<0>(ops_partials).partials_ = (ntheta > cutoff) .select( -exp_m_ntheta, // <-- bug (ntheta >= -cutoff) .select(signs * exp_m_ntheta / (exp_m_ntheta + 1), signs));For
ntheta > cutoffthe value is−exp(−ntheta), sod(value)/d(ntheta) = +exp_m_ntheta, and the chain rule throughntheta = signs · theta(signs constant in theta) gives∂lp/∂theta = signs · exp_m_ntheta. The upper partials branch returns−exp_m_ntheta— the derivative of the value without the chain-rule factor. It is correct only whensigns = −1(n = 0). Forn = 1withtheta > 20(and, symmetrically,n = 0withtheta < −20) the gradient of every such element has the wrong sign.Eval
cutoff_partials_sign(test/unit/math/prim/prob/bernoulli_logit_test.cpp): for bothn = 1, theta = +25andn = 0, theta = −25(both put ntheta above the cutoff) it checks the autodiff gradient against (a) the analyticsigns * exp(−ntheta)and (b) central finite differences of the double implementation, h = 1e-3 (small enough that both FD points stay inside the same branch, large enough that the ~1e-11-magnitude values subtract cleanly).2·exp(−ntheta)) and passes with the fix. Full test binary 6/6; the file's existing value-space cutoff test is unaffected — the bug is in partials only.Checklist
Copyright holder: Maximilian Scholz
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