-
-
Notifications
You must be signed in to change notification settings - Fork 94
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/659 diagnose verbose #684
Conversation
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.
@mitzimorris, some basic things:
- who owns the copyright?
- could you please update the issue (or the PR) to say what this does? I had to fire up everything to figure out that the output looks like:
Processing csv files: 1.csv, 2.csv, 3.csv, 4.csv, 5.csv
All transitions within treedepth limit.
No divergent transitions.
Processing complete.
- There are no line breaks in the message if there's a divergence? This just looks weird.
- Did you want a summary in the
processing complete
line? Or at least an "exiting with error" or something?
made requested changes to program output. |
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'd prefer this to be set up so that each test provides a warning/error or an "all's well" output.
Then the has_errors
flag isn't necessary and the "all's well" messages can just be moved up as else
conditions on the relevant tests. The doc would need to change a bit, too.
For some reason, I didn't see that @syclik had already started reviewing this. So take my input as an optional suggestion. I just approved my change request so it won't block anything. |
@syclik - done making changes - please review. discussed Rhat msg with Ben and Aki at today's Stan meeting. using constant to allow for future changes. |
…ature/659-diagnose-verbose
@syclik - are messages OK? |
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.
Thanks!
Sorry--I missed the original notification. Please feel free to ping me again if that happens. It's approved. |
@syclik - thanks! |
Submisison Checklist
./runCmdStanTests.py src/test
Summary:
Added output stmts for:
Intended Effect:
Always provide output for command - if diagnose command doesn't find problems, then it will print the following message:
How to Verify:
unit tests
Side Effects:
none.
Documentation:
updated cmdstan user's guide accordingly
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: