jacobian returns wrong results in some situations

[wds15]

Summary:

The jacobian function returns in special situations the wrong gradients.

Description:

The jacobian function in Stan uses the nested AD system to calculate gradients. As such the

set_zero_all_adjoints_nested();

function is used. Apparently, this leads to wrong results if the AD stack was never initialized before. So results are wrong if no stan::math::var was declared before the jacobian call. Replacing calls to set_zero_all_adjoints_nested(); with set_zero_all_adjoints(); seem to make the bug go away.

Reproducible Steps:

I am sending a cpp file to the dev list which shows the issue. When one uncomments the dummy definition of a var instance, then results are correct.

Current Output:

Current output depends on what happened before the call as described.

Expected Output:

Results should not depend on a first declaration of a dummy variable.

Additional Information:

Reported on stan-users from Oleg Shirokobrod.

Current Version:

develop

[bob-carpenter]

My bad for not checking this closer on the first commit.

recover_memory_nested() calls need to be matched to start_nested() calls as in stan/math/rev/mat/functor/gradient.hpp. It's just like malloc and free or new and delete. But now that I look at the Jacobian code, it looks like that's what's going on, at least in the develop version.

I also edited the issue to include the code.

[bob-carpenter]

I went and looked at the actual implementation and it does have start_nested(). No idea why transposing the output produces an issue; were there changes other than reordering the output?

[wds15]

Hi Bob!

The change which seems to cause this problem is going from (old)

set_zero_all_adjoints();
to

set_zero_all_adjoints_nested();

In the patch which corrected for the transposition I started to use the _nested version of set_zero_all_adjoints. If you replace the set_zero_all_adjoints_nested(); with set_zero_all_adjoints(); then all is fine again which is why I am suspecting some issue there.

BTW, I had to adjust the test in test/unit/math/rev/mat/fun/jacobian.hpp to also declare a dummy variable like I explained in the other thread. I guess that got lost.

So if someone could check the set_zero_all_adjoints_nested() function which apparently does not work correctly for the case when the AD stack is empty. This would be my diagnosis from this.

Thanks for reediting the issue, I tried to drop the cpp file here, but this is not possible. Code blocks are of course even better. Next time.

[bob-carpenter]

Thanks. That should help diagnose what's going on. I reassigned to myself. I'll try to also check that the Hessians and other higher-order derivatives are doing the right thing, because they also use nesting.

Using set_zero_all_adjoints() isn't improper, just inefficient when you're doing nesting; so at worst, we can go back to that for now if there's not a deeper fix before the next release.

[wds15]

Thanks Bob for taking over. Yes, we can simply use the set_zero_all_adjoints() in the jacobian function. Should I start this branch by reconfiguring the test I was referring to such that it will now fire a failure? Then you could take over and either fix the set_zero_all_adjoints_nested() or switch to set_zero_all_adjoints() in the jacobian function. Makes sense?

[syclik]

Yes, that would help a lot. Thanks!

On Apr 7, 2016, at 5:04 AM, wds15 notifications@github.com wrote:

Thanks Bob for taking over. Yes, we can simply use the set_zero_all_adjoints() in the jacobian function. Should I start this branch by reconfiguring the test I was referring to such that it will now fire a failure? Then you could take over and either fix the set_zero_all_adjoints_nested() or switch to set_zero_all_adjoints() in the jacobian function. Makes sense?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

[wds15]

Ok, you got it. The test

test/unit/math/mix/mat/functor/autodiff_test.cpp

on the branch bugfix/issue-278-jacobian-nested now fails.

[wds15]

I am confused that the travis run is ok. On my machine I get

test/unit/math/mix/mat/functor/autodiff_test.cpp:135: Failure
Value of: J(1,0)
  Actual: -5
Expected: -9
[  FAILED  ] AgradAutoDiff.jacobian (0 ms)

when runing ./runTests.py test/unit/math/mix/mat/functor/autodiff_test.cpp

[bob-carpenter]

I have a hunch as to what's going on, and will try to fix
it today.

• Bob

On Apr 8, 2016, at 5:34 AM, wds15 notifications@github.com wrote:

I am confused that the travis run is ok. On my machine I get

test/unit/math/mix/mat/functor/autodiff_test.cpp:135: Failure
Value of: J(1,0)
Actual: -5
Expected: -9
[ FAILED ] AgradAutoDiff.jacobian (0 ms)

when runing ./runTests.py test/unit/math/mix/mat/functor/autodiff_test.cpp

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub