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

Issue/3200 pathfinder diagnostic #3202

Merged
merged 52 commits into from
Jul 10, 2023
Merged

Conversation

mitzimorris
Copy link
Member

@mitzimorris mitzimorris commented Jun 30, 2023

Submission Checklist

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

Summary

Add hooks to json_writer to put newlines around records and update single-path pathfinder accordingly.

  • begin_record takes optional arg "newline"; if true, will output newline before record start or key.
  • end_record takes optional arg "newline"; if true, puts newline after record end, furthermore, final close record will be on its own line.

Also got rid of all calls to boost::lexical_cast - see #3201

Intended Effect

Produce readable-ish JSON; output file will be properly terminated, diagnostics from each iteration are on their own line.

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:

@mitzimorris mitzimorris requested review from SteveBronder, bob-carpenter and WardBrian and removed request for bob-carpenter June 30, 2023 22:23
@mitzimorris
Copy link
Member Author

is there a pathfinder unit test that checks output files? if so, can add tests for single-path pathfinder.

@WardBrian
Copy link
Member

How ugly is it to always emit newlines after closing records? I feel like adding a flag for this is not the cleanest solution

@mitzimorris
Copy link
Member Author

mitzimorris commented Jun 30, 2023

we would have to add "write_newline" to json_writer. also a fine way to go and much much simpler and would probably improve code readability, plus flexibility.

shall I re-implement?

@mitzimorris
Copy link
Member Author

I realize that breaking iterations out by line is completely unneccessary. the minimal fix for this PR is just adding the "newline" method, no fanciness w/r/t comma between records.

if the reviewers prefer, I can simplify the PR and leave the diagnostic JSON as is, simply adding a newline at the end.

@WardBrian
Copy link
Member

I think I prefer simpler. I wouldn’t be opposed to automatically adding new lines at the end of lists/records, if that produces desirable output, but doing it case by case seems clunky

@mitzimorris
Copy link
Member Author

simplified. will try to remember this for next time.

src/stan/callbacks/json_writer.hpp Outdated Show resolved Hide resolved
src/stan/io/dump.hpp Outdated Show resolved Hide resolved
src/stan/io/dump.hpp Outdated Show resolved Hide resolved
src/stan/callbacks/json_writer.hpp Show resolved Hide resolved
@WardBrian WardBrian requested a review from SteveBronder July 10, 2023 02:14
Copy link
Collaborator

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@mitzimorris mitzimorris merged commit 93617dc into develop Jul 10, 2023
@WardBrian WardBrian deleted the issue/3200-pathfinder-diagnostic branch July 10, 2023 15: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.

6 participants