Off by one error in `set_zero_all_adjoints_nested`

[bbbales2]

Description

I think there's a bug in set_zero_all_adjoints_nested where we zero one vari from the stack above.

Test code is:

TEST(AgradRev, chainable_object_nested_testadj) {
  stan::math::var a = 1.0;
  a.adj() = 2.0;

  stan::math::start_nested();
  EXPECT_FLOAT_EQ(a.adj(), 2.0);
  stan::math::set_zero_all_adjoints_nested();
  EXPECT_FLOAT_EQ(a.adj(), 2.0);
  stan::math::recover_memory_nested();
  EXPECT_FLOAT_EQ(a.adj(), 2.0);
}

Fix is to not do the - 1 in the set_zero_all_adjoints_nested code:

const size_t start1
      = ChainableStack::instance_->nested_var_stack_sizes_.back();
  // avoid wrap with unsigned when start1 == 0                                                                                                                                                
  for (size_t i = (start1 == 0U) ? 0U : (start1 - 1); // <-- bug is here I think
       i < ChainableStack::instance_->var_stack_.size(); ++i) {
    ChainableStack::instance_->var_stack_[i]->set_zero_adjoint();
  }

Current output is:

Running main() from lib/benchmark_1.5.1/googletest/googletest/src/gtest_main.cc
[==========] Running 3 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 3 tests from AgradRev
[ RUN      ] AgradRev.chainable_object_test
[       OK ] AgradRev.chainable_object_test (0 ms)
[ RUN      ] AgradRev.chainable_object_nested_test
[       OK ] AgradRev.chainable_object_nested_test (0 ms)
[ RUN      ] AgradRev.chainable_object_nested_testadj
test/unit/math/rev/core/chainable_object_test.cpp:52: Failure
Expected equality of these values:
  a.adj()
    Which is: 0
  2.0
    Which is: 2
test/unit/math/rev/core/chainable_object_test.cpp:54: Failure
Expected equality of these values:
  a.adj()
    Which is: 0
  2.0
    Which is: 2
[  FAILED  ] AgradRev.chainable_object_nested_testadj (1 ms)

@wds15 do you know the reason for these minus 1s?

Current Version:

v3.4.0

[wds15]

I overlooked this... no I don't know the reason for the -1. Is this is still ongoing?

[bbbales2]

Just tested it. It still happens yeah.

[wds15]

Hmm...if my memory serves me right then @bob-carpenter could have done this change in response to a bug report from mine; but I am blank on that. Maybe @bob-carpenter recalls the context? If not, then we should probably apply your suggested fix which sounds sensible to me.

[nhuurre]

if my memory serves me right

git blame confirms: issue #278, pull #280

[wds15]

looks to me as if it would still be ok to not do the -1 thing unless I am overlooking something. Thanks @nhuurre for digging that up.