-
-
Notifications
You must be signed in to change notification settings - Fork 370
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
Bugfix/3301 stan csv reader skip warmup #3302
Conversation
@WardBrian - Ubuntu tests failing because of nodejs warning?
the offending test uses the Stan CSV reader - but works on Mac. is there something cached that needs cleanup or???? |
The node warning is not the source of the error, its
There is also a separate failure in Jenkins:
|
Looking at the laplace test, it is setting up a writer without a comment prefix and then later feeding that back into the csv_reader, which is now expecting to find |
I just fixed that one - checked in a test file. |
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: |
that upstream test failure is because now "fixed params" outputs aren't parsing. update: issue exists since 3 years: stan-dev/cmdstan#1029 |
I am still not sure I follow what the goal of this PR is -- is there a bug it is fixing? |
This will make the returned stan_csv object correctly capture the adaptation information in the case that the Stan CSV file has saved warmup draws - that's technically a bug, no? It will also ensure that the stan_csv object's It will also simplify the logic in the |
Ah, I missed how this would fix reading adaptation. That is nice to have. How positive are we that nobody uses the warmup draws read in by this class? I noticed this class is imported at least once in RStan |
Imported, but no longer used? All the diagnostics were rewritten circa 2019 when the improved Rhat paper came out. |
I filed stan-dev/cmdstan#1286 on CmdStan which would make deciding when to skip warmup more straightforward. |
fixed by #3311 |
Submission Checklist
./runTests.py src/test/unit
make cpplint
Summary
Added logic to skip warmup draws, if any present, more checks on consistency between config and output.
Intended Effect
see #3301
How to Verify
Unit tests
Side Effects
N/A
Documentation
N/A
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: