Is this an ok solution to make this test pass?

bugs

#21

I went through my code to see if there was a way to break things up but I can’t add tests first since the current code fails under Bob’s auto-diff test framework and if I introduce any of my code the old tests fail (for some reason I think the actual reference values are not accurate in the old tests). Is there anything you could do to narrow down the memory issue you pointed out? This one:

I could take a look at it if you give me a place to start but there’s not enough info in your message to figure out what you found.


#22

Sorry about that – I didn’t actually commit any of it yet. This is the first thing I’m doing on Stan this week.

Btw, mind recreating a PR for that branch?


#23

I’ve just updated the branch:

  • reverted the revert commit (using git revert)
  • reverted the change to the test (as a separate commit)

#24

For anyone following along, this will make and run just the test that fails. It helps to get quicker access to results:

make test/prob/inv_chi_square/inv_chi_square_ccdf_log_00000_generated_ffv_test; ./test/prob/inv_chi_square/inv_chi_square_ccdf_log_00000_generated_ffv_test --gtest_filter=AgradCcdfLogInvChiSquare_ffv_9/AgradCcdfLogTestFixture/0.Function

#25

@sakrejda, sorry, but there are legitimate issues in the gamma_p implementation. I’m coming up with a minimal example. Stay tuned.


#26

Np, if there’s a real problem to look at I’m sure I can fix it.


#27

Just pop this into a test somewhere:

#include <stan/math/mix/mat.hpp>
#include <gtest/gtest.h>

TEST(test, works) {
  using stan::math::fvar;
  using stan::math::var;
  using std::vector;

  fvar<fvar<var>> y = 0.17;
  fvar<fvar<var>> fy = gamma_p(0.25, y);

  vector<double> grad;
  vector<var> x;
  x.push_back(y.val_.val_);
  fy.val_.val_.grad(x, grad);
  stan::math::recover_memory();

  ASSERT_EQ(1, grad.size());
  EXPECT_FALSE(stan::math::is_nan(grad[0]))
      << "  - grad[0] = " << grad[0];
  EXPECT_FLOAT_EQ(0.878926, grad[0]);
}

TEST(test, fails) {
  using stan::math::fvar;
  using stan::math::var;
  using std::vector;

  fvar<fvar<var>> y = 0.17;
  fvar<fvar<var>> fy = 1.0 - gamma_p(0.25, y); // this fails

  vector<double> grad;
  vector<var> x;
  x.push_back(y.val_.val_);
  fy.val_.val_.grad(x, grad);
  stan::math::recover_memory();

  ASSERT_EQ(1, grad.size());
  EXPECT_FALSE(stan::math::is_nan(grad[0]))
      << "  - grad[0] = " << grad[0];
  //EXPECT_FLOAT_EQ(, grad[0]);
}

#28

I haven’t figured out exactly what’s going on to create that NaN, but it’s clearly not right.


#29

Ooh, I have some guesses but no computer at the moment. If you don’t see an obvious solution let me take a look before you sink time into it.


#30

I don’t see anything obvious. If it were, I’d have fixed it! =)

Damn, I’m glad we have these tests.


#31

I’m guessing it’s either the more limited type promotion or the boundaries between implementations but I’ll check.


#32

Found it! It wasn’t subtle :(


#33

Turns out adj_ can be negative… which… yeah… makes sense… so you shouldn’t log it… all the direct-input parameters were positive so I got lulled into a false sense of security about logs…


#34

Wow. How’d you wind up taking the log of a negative? I’d have thought that wouldn’t come up with an ordinary function’s gradients.


#35

Why? Are you saying adj_ should never be negative?


#36

No. I’d have thought that if you were calculating gradients using the chain rule that you wouldn’t wind up taking the log of a negative number. Are you writing the actual derivative code yourself?


#37

Yeah, this is in rev, there was something like adj_ * exp(a)/tgamma(b) and I replaced it with exp(log(adj_) + a - lgamma(b)) to make some more digits of accuracy and make tests pass but it should’ve just been adj_ * exp(a - lgamma(b)). Having chased down the rest of the bugs and introduced automatic testing I’d have to go back and doublecheck that it’s necessary.

IIRC… I preferred lgamma over tgamma in this code b/c sometimes tgamma would go to inf when lgamma doesn’t. It’s often just the denominator that has tgamma so it’s better to let the final expression overflow to zero rather than throwing the whole calculation due to an inf.


#38

I’d think that’d be much more stable. tgamma has a pretty limited domain.