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

Exception in transformed parameters stops sampling #2562

Closed
syclik opened this issue Jun 27, 2018 · 19 comments · Fixed by #2570
Closed

Exception in transformed parameters stops sampling #2562

syclik opened this issue Jun 27, 2018 · 19 comments · Fixed by #2570
Assignees
Labels
Milestone

Comments

@syclik
Copy link
Member

syclik commented Jun 27, 2018

Summary:

An exception in transformed parameters stops sampling from continuing. It should catch the exception and continue to the next iteration, if possible.

Description:

When constraints are violated, it seems like the exception terminates Stan. We should catch it and move to the next iteration. It's odd that it doesn't print the messages to the logger. Perhaps there's some buffering happening.

Reproducible Steps:

Here's a Stan program:

parameters {
  real<lower = 0, upper = 1> theta;
}
transformed parameters {
  real<lower = 0.9, upper = 1> alpha;

  alpha = theta;
  print("theta = ", theta);
  print("alpha = ", alpha);
}
model {
}

Current Output:

From CmdStan. Header information then:

Exception: issue_2562_model_namespace::write_array: alpha is 0.827004, but must be greater than or equal to 0.9  (in 'issue-2562.stan' at line 5)

Expected Output:

From CmdStan. I'd expect header information, then

theta = 0.2
alpha = 0.2
<some message about an issue with transformed parameters>
theta = 0.8
alpha = 0.8
<some message about an issue with transformed parameters>

and if we got lucky and theta was drawn in the 0.9 to 1.0 range, it'd proceed as expected.

Additional Information:

I'm seeing slightly different behavior in CmdStan and RStan, so I don't know what's up. I think this is probably being handled improperly by Stan, though.

Current Version:

v2.17.1

Edits

5/27. Daniel. Added reproducible example.

@syclik syclik added the bug label Jun 27, 2018
@syclik
Copy link
Member Author

syclik commented Jun 27, 2018

Stan v2.17.0 is correct (through CmdStan). This is what it looks like:

...
Rejecting initial value:
  Error evaluating the log probability at the initial value.
validate transformed params: alpha is 0.473356, but must be greater than or equal to 0.9
theta = 0.191311
alpha = 0.191311

Rejecting initial value:
  Error evaluating the log probability at the initial value.
validate transformed params: alpha is 0.191311, but must be greater than or equal to 0.9
theta = 0.569988
alpha = 0.569988

Rejecting initial value:
  Error evaluating the log probability at the initial value.
validate transformed params: alpha is 0.569988, but must be greater than or equal to 0.9
theta = 0.359322
alpha = 0.359322

Rejecting initial value:
  Error evaluating the log probability at the initial value.
validate transformed params: alpha is 0.359322, but must be greater than or equal to 0.9
theta = 0.234157
alpha = 0.234157

Rejecting initial value:
  Error evaluating the log probability at the initial value.
validate transformed params: alpha is 0.234157, but must be greater than or equal to 0.9

Initialization between (-2, 2) failed after 100 attempts. 
 Try specifying initial values, reducing ranges of constrained values, or reparameterizing the model.
Initialization failed.

Stan v2.17.1 (and using develop) is not correct:

Exception: issue_2562_model_namespace::write_array: alpha is 0.672169, but must be greater than or equal to 0.9  (in 'issue-2562.stan' at line 5)

So... we've got to hunt down what's caused this and fix it.

@syclik
Copy link
Member Author

syclik commented Jun 27, 2018

Does anyone know if the exception type changed in write_array?

@bob-carpenter
Copy link
Contributor

bob-carpenter commented Jun 27, 2018 via email

@syclik
Copy link
Member Author

syclik commented Jun 27, 2018

I verified that there was no change to exception types. But I did see a lot of changes to src/stan/services/util/initialize.hpp. I think this behavior was changed in there.

@syclik
Copy link
Member Author

syclik commented Jun 27, 2018

@mitzimorris, I found an error in the generated code. Inside write_array(), it currently looks like (a lot of stuff omitted):

// validate transformed parameters
current_statement_begin__ = 5;
check_greater_or_equal(function__,"alpha",alpha,0.90000000000000002);
check_less_or_equal(function__,"alpha",alpha,1);
  
// write transformed parameters
if (include_tparams__) {
  vars__.push_back(alpha);
}

The problem is that it's throwing within the section that's commented // validate transformed parameters.

To line up with the existing behavior, it should be within the if (include_tparams__) block. So something like this:

if (include_tparams__) {
  // validate transformed parameters
  current_statement_begin__ = 5;
  check_greater_or_equal(function__,"alpha",alpha,0.90000000000000002);
  check_less_or_equal(function__,"alpha",alpha,1);

  // write transformed parameters
  vars__.push_back(alpha);
}

@syclik
Copy link
Member Author

syclik commented Jun 27, 2018

There's an alternate fix. If we think the model shouldn't be instantiated when it violates the transformed parameters block, we need to do something a little more complicated. I think that makes sense.

@bob-carpenter, think that's right? The log probability function should throw an exception if the constrained values used result in transformed parameters (or even generated quantities) that violate constraints EVEN IF the arguments to the functions that say to include it is set to false?

@syclik
Copy link
Member Author

syclik commented Jun 27, 2018

The alternate way to fix this is to change initialize.hpp in the initialize() function within the for loop. We can catch on construction of the random_context() which would indicate that the transformed parameters aren't valid. (We'd have to make the assumption that it won't error for any other condition, which I think is a safe assumption for now.)

@mitzimorris
Copy link
Member

used git blame to look at code for initialize.hpp and it pointed me to this commit:
5b92f49
which changed the behavior of mcmc_writer.hpp

is this relevant?

@syclik
Copy link
Member Author

syclik commented Jun 27, 2018

No, I tracked it down to a change in the generated code.

If you look at it in v2.17.0 vs v2.17.1 you'll see that there's an

if (include_tparams__) return;

much higher in the code in v2.17.0 and it's missing in v2.17.1; right before validating the transformed parameters.

So, the behavior of Stan programs have changed slightly now. Before, if you could evaluate the log probability and include_tparams__ == false, then you'd be able to evaluate the log probability function without a problem, even if the transformed parameter constraints were violated. Now, if include_tparams__ == false, it will throw an exception if any of the transformed parameters constraints are violated. It also happens when include_tparams__ == true, but the difference in behavior is when include_tparams__ == false.

@bob-carpenter
Copy link
Contributor

Shoudln't that be an if (!include_tparams__)? Why return if including them?

@bob-carpenter
Copy link
Contributor

Also, I think the right behavior should be that transformed parameter constraint violations should throw exceptions no matter whether they're being included in the output or not.

@syclik
Copy link
Member Author

syclik commented Jun 27, 2018 via email

@syclik
Copy link
Member Author

syclik commented Jun 27, 2018 via email

@mitzimorris
Copy link
Member

change was introduced as part of standalone generated quantities - 1ca2236

question: if you don't include transformed parameters, then how can you run standalone gqs?

@bob-carpenter
Copy link
Contributor

The transformed parameters need to generated in order to generate the standalone generated quantities. But they don't need to be output.

@seantalts seantalts added this to the 2.17.1++ milestone Jul 6, 2018
@seantalts
Copy link
Member

Is there a PR for this? Who is working on it? Not sure the best way to label this in the github UI, but @syclik has said this is a blocker for 2.18.

@syclik
Copy link
Member Author

syclik commented Jul 6, 2018 via email

@mitzimorris
Copy link
Member

@syclik - happy to help out on generator code logic.

@syclik
Copy link
Member Author

syclik commented Jul 6, 2018 via email

@syclik syclik self-assigned this Jul 6, 2018
syclik added a commit that referenced this issue Jul 6, 2018
syclik added a commit that referenced this issue Jul 7, 2018
… exception behavior in new generated code.
syclik added a commit that referenced this issue Jul 7, 2018
syclik added a commit that referenced this issue Jul 7, 2018
… exception behavior in new generated code.
syclik added a commit that referenced this issue Jul 7, 2018
… exception behavior in new generated code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants