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
750 stars 189 forks source link

fix Jacobian to return transpose #230

Closed bob-carpenter closed 8 years ago

bob-carpenter commented 8 years ago

Sebastian Weber on stan-users pointed out that the matrix returned by the Jacobian functional in math/rev/mat/functor/jacobian.hpp returns a tranposed result.

According to

https://en.wikipedia.org/wiki/Jacobian_matrix_and_determinant

if f: R^N -> R^M, then the Jacobian should be M x N

But it's currently N x M.

wds15 commented 8 years ago

Hi!

Going over the function again, I think there is another bug, i.e. the function calls to

set_zero_all_adjoints();

should possibly be replaced with calls to

set_zero_all_adjoints_nested();

Right?

Sebastian

bob-carpenter commented 8 years ago

Either way will be OK, but the nested version will be more efficient (the stuff below the current nesting will already have all its adjoints set to zero because they're only nonzero in reverse sweeps).

On Feb 20, 2016, at 5:51 AM, wds15 notifications@github.com wrote:

Hi!

Going over the function again, I think there is another bug, i.e. the function calls to

set_zero_all_adjoints();

should possibly be replaced with calls to

set_zero_all_adjoints_nested();

Right?

Sebastian

— Reply to this email directly or view it on GitHub.

wds15 commented 8 years ago

Hi Bob!

Let' start with smaller patches... I just fixed the problem described and pushed the branch

bugfix/issue-230-jacobian-transposed

Maybe you have a look and then we start a pull request?

Sebastian

syclik commented 8 years ago

@wds15, please create a pull request from that branch to develop. Thank you.