Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

propto doesn't work when there are no autodiff types #1020

Closed
bbbales2 opened this issue Sep 4, 2018 · 4 comments
Closed

propto doesn't work when there are no autodiff types #1020

bbbales2 opened this issue Sep 4, 2018 · 4 comments
Labels
Milestone

Comments

@bbbales2
Copy link
Member

bbbales2 commented Sep 4, 2018

Description

This issue follows up a problem @yao-yl found and posted here: https://discourse.mc-stan.org/t/dropping-the-constant-in-log-prob/3889/6

This function returns the log density of a normal:

stan::math::normal_log<false>(0, mu1, 1);

This function returns the log density of a normal up to a proportionality constant:

stan::math::normal_log<true>(0, mu1, 1);

The difference between the two should remain constant as a function of parameters (otherwise the proportionality constant isn't constant). If mu1 is a double, the difference between the two changes as mu1 takes different values (this is presumeably incorrect behavior).

If mu1 is a var (and presumeably any other autodiff type), it works correctly.

This is because the question of 'proportional to what' is baked in with are the variables autodiff types or not.

I'm pretty sure this is by design. Maybe we can have a wontfix label for this one? I assume it should be documented somewhere, but where might that be? I think it basically comes down to propto only works for autodiff types and should otherwise be set to false.

Example

Add this code to test/unit/math/rev/scal/prob/normal_log_test.cpp

TEST(ProbDistributionsNormal, propto) {
  using stan::math::var;

  double mu1 = 1.0;
  double mu2 = 2.0;

  double d1 = stan::math::normal_log<true>(0, mu1, 1) - stan::math::normal_log<false>(0, mu1, 1);
  double d2 = stan::math::normal_log<true>(0, mu2, 1) - stan::math::normal_log<false>(0, mu2, 1);

  double v1 = stan::math::value_of(stan::math::normal_log<true>(0, var(mu1), 1) - stan::math::normal_log<false>(0, var(mu1), 1));
  double v2 = stan::math::value_of(stan::math::normal_log<true>(0, var(mu2), 1) - stan::math::normal_log<false>(0, var(mu2), 1));

  EXPECT_FLOAT_EQ(v1, v2);
  EXPECT_FLOAT_EQ(d1, d2);
}

And run ./runTests.py test/unit/math/rev/scal/prob/normal_log_test.cpp

Expected Output

It should output no errors. Instead the output is:

[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from ProbDistributionsNormal
[ RUN      ] ProbDistributionsNormal.intVsDouble
[       OK ] ProbDistributionsNormal.intVsDouble (0 ms)
[ RUN      ] ProbDistributionsNormal.propto
test/unit/math/rev/scal/prob/normal_log_test.cpp:38: Failure
Value of: d2
  Actual: 2.9189386
Expected: d1
Which is: 1.4189385
[  FAILED  ] ProbDistributionsNormal.propto (0 ms)
[----------] 2 tests from ProbDistributionsNormal (0 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (0 ms total)
[  PASSED  ] 1 test.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] ProbDistributionsNormal.propto

Which indicates that the proportionality constant is not constant between parameter values.

Current Math Version

v2.18.0

@syclik
Copy link
Member

syclik commented Sep 4, 2018

What you said is correct. This is a design decision.

Something else you'll find is the first term in both d1 and d2 are exactly 0.

What do you think we should do? It's described pretty thoroughly in the Stan Math arxiv paper. https://arxiv.org/abs/1509.07164. Should we add it somewhere on the wiki and close this issue?

@bbbales2
Copy link
Member Author

bbbales2 commented Sep 5, 2018

Hmm, let's just close this issue and I'll add a note to the Discourse thread. It'll get search engine indexed and so hopefully comes up if anyone searches for propto and Stan. It's just something we gotta be aware of.

@syclik
Copy link
Member

syclik commented Sep 5, 2018 via email

@bbbales2
Copy link
Member Author

bbbales2 commented Sep 5, 2018

Yeah, just an easy confusion. I think what's here'll be fine. Not an issue that'll come up every day I don't suppose.

@bbbales2 bbbales2 closed this as completed Sep 5, 2018
@syclik syclik added this to the 2.19.0 milestone Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants