-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
Feature/3299 chainset #3313
Feature/3299 chainset #3313
Conversation
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is still marked as draft, but most of my concerns are in the tests, so I hope this is still helpful at this stage
I am not convinced that they are expected (I recently wrote a javascript version of ESS and RHat that matches stan's current C++ out to ~10 digits). I would want to see some explanation of exactly where and why they do to this degree, if we're claiming to implement the same things. But, if these differences can be justified, then we shouldn't be testing against those values at all. The presence of those values in the tests visually implies we do match the R values (when we don't), and the resulting tests have very little power to catch issues. For example:
I pick this example not just to be a pain, but because replacing the in-line standard deviation calculation in mcse_mean function is something a future code editor may be likely to do (if we don't already change it during review for this!), and accidentally picking the wrong library function is not hard to imagine, so failing the tests if that mistake gets made would be very helpful. Similar arguments can be made for basically all the other tests having tighter tolerances |
I would be happy to find another way to test these. what would you suggest? |
What previous implementations of diagnostics seem to do is just run the code once to get values and then "freeze" those in the test. I think #3312 deleted some tests in that style. Doing this requires us to be pretty certain the implementation is correct, since we're making it the gold standard for all future versions of the code, which is why it would be important to understand what parts of the calculation are different than R |
perhaps we should create an artificial dataset to convince ourselves of the goodness of ESS and Rhat. what did you use for Javascript? |
For testing I ended up using the same blocker1.csv and blocker2.csv as the stan tests, but for initially getting them to agree I was just using an arbitrary CSV file, I think from the Bernoulli model? |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
The unit tests now test basic ESS and Rhat against the values computed by CmdStan 2.35.0's bin/stansummary and the saved summary CSV file has been checked into the folder src/test/unit/analyze/mcmc/test_csv_files. I have added unit tests for rank-normalization and splitting the chains - the latter did uncover a bug where splitting a chain with an odd number of draws would omit the last draw, not the (N+1)/2 draw; fixed - but this bug would not have been tickled by any of the existing tests since all CSV test sets had and even number of draws. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things I'd like to see before merge but then I think this can fly. Thank you for the diligence!
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Submission Checklist
./runTests.py src/test/unit
make cpplint
Summary
This is a do-over of PR #3310. The overall goal is to make the improved R-hat diagnostics (#3266), and rank-normalized ESS bulk and tail, (#3312) available to CmdStan, following the definitions in https://arxiv.org/abs/1903.08008.
This PR adds a new class
stan::mcmc::chainset
. Achainset
object requires that all chains are the same size on construction, unlike thechains<>
object. This simplifies computation of split-Rhat and split-ESS diagnostics. Thechainset
object provides rank-normalized split Rhat, rank-normalized split ESS which returns bulk and tail ESS, and functionsmcse
andmcse_sd
which calculate the mean Monte Carlo standard error and its variance.How to Verify
Unit tests for the chainset test performance against Stan CSV outputs. The CmdStan branch https://github.com/stan-dev/cmdstan/tree/feature/1263-new-rhat-summary further checks that
chainset
provides the necessary functions to output the same set of summary statistics as the R interfaces.Side Effects
N/A
Documentation
Documentation will be added to the Stan docset in a separate PR.
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Columbia University
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: