search

stan-dev / math

The Stan Math Library is a C++ template library for automatic differentiation of any order using forward, reverse, and mixed modes. It includes a range of built-in functions for probabilistic modeling, linear algebra, and equation solving.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
737 stars 185 forks source link

softmax setting adjoint instead of incrementing adjoint #2048

Closed bbbales2 closed 4 years ago

bbbales2 commented 4 years ago

Description

@SteveBronder found that softmax isn't quite working right. The reverse mode softmax code here sets the adjoint instead of incrementing it: https://github.com/stan-dev/math/blob/develop/stan/math/rev/fun/softmax.hpp#L39

Changing the = to += fixes the problem.

I think this could be tested in the autodiff test framework. The test would be set the adjoints of the inputs to something non-zero, do the reverse mode pass, and then check the adjoints have been incremented by what you'd expect if the adjoints had been zero.

If this is fixed without the more general fix to the autodiff tester, make a new issue for that.

Current Version:

v3.3.0

bob-carpenter commented 4 years ago

more general fix to the autodiff tester

+1 to that. The testing for nested use (where you need to increment) is sketchy at best, but could be easily incorporated into the test framework.