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

Remove duplicate acceptance procedure, update tests #1979

Merged
merged 2 commits into from Jul 27, 2016

Conversation

betanalpha
Copy link
Contributor

Submisison Checklist

  • Run unit tests: ./runTests.py src/test/unit
  • Run cpplint: make cpplint
  • Declare copyright holder and open-source license: see below

Summary

Removes duplicate acceptance procedure in the default NUTS algorithm that was causing chains to push too far out into the tails.

Intended Effect

Restores correctness of default NUTS algorithm.

How to Verify

The following model,

parameters {
  real<lower=0, upper=1> x;
}
model {
  x ~ uniform(0, 1);
}

should now return an actually uniform distribution.

Side Effects

Small changes to all default sampler output.

Documentation

None.

Reviewer Suggestions

@syclik, @bob-carpenter, @bgoodri

Copyright and Licensing

Copyright: University of Warwick.

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@betanalpha
Copy link
Contributor Author

No idea what's going on with Travis -- and where is Jenkins?

@syclik
Copy link
Member

syclik commented Jul 26, 2016

Code looks good. Just verified it fixes the model in question.

@syclik syclik added this to the v2.10.0++ milestone Jul 26, 2016
@betanalpha
Copy link
Contributor Author

Yeah, it’ll fix the uniform model and the model Ben has introduced a few
weeks ago. The logistic test will fail, however, so I need Jenkins to run
to get the Windows-specific values to update the hard-coded values.

On Jul 26, 2016, at 2:48 PM, Daniel Lee notifications@github.com wrote:

Code looks good. Just verified it fixes the model in question.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@syclik
Copy link
Member

syclik commented Jul 26, 2016

Those values are on Mac OS X, so you should be able to run locally.

@betanalpha
Copy link
Contributor Author

They’ve never translated before — the values I get locally would
be very slightly off from what Jenkins reported. Has that test moved
to an OS X recently?

On Jul 26, 2016, at 3:05 PM, Daniel Lee notifications@github.com wrote:

Those values are on Mac OS X, so you should be able to run locally.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@syclik
Copy link
Member

syclik commented Jul 26, 2016

Nope. They've always been Mac OS X tests.

On Tue, Jul 26, 2016 at 10:08 AM, Michael Betancourt <
notifications@github.com> wrote:

They’ve never translated before — the values I get locally would
be very slightly off from what Jenkins reported. Has that test moved
to an OS X recently?

On Jul 26, 2016, at 3:05 PM, Daniel Lee notifications@github.com wrote:

Those values are on Mac OS X, so you should be able to run locally.


You are receiving this because you authored the thread.

Reply to this email directly, view it on GitHub, or mute the thread.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1979 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAZ_FyqHDv-8Y4cTmdqYZi4MHnEKjb3fks5qZhTGgaJpZM4JVEiQ
.

@syclik syclik merged commit afb77b5 into develop Jul 27, 2016
@syclik syclik deleted the bugfix/issue-1972-remove_double_acceptance branch July 27, 2016 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants