stan-dev / stan

Stan development repository. The master branch contains the current release. The develop branch contains the latest stable development. See the Developer Process Wiki for details.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
2.56k stars 366 forks source link

Issue/3200 pathfinder diagnostic #3202

Closed mitzimorris closed 1 year ago

mitzimorris commented 1 year ago

Submission Checklist

Summary

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

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 commented 1 year ago

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

WardBrian commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

re-implemented, added function "newline" with optional argument "add_comma".

this isn't particularly pretty, but it allows the single-path pathfinder to create JSON files one iteration per line.

{
"0" : {"iter" : 0, "unconstrained_parameters" : [ -1.55164 ], "grads" : [ 0.901806 ]},
"1" : {"iter" : 1, "unconstrained_parameters" : [ -1.55164 ], "grads" : [ 0.901806 ], "history_size" : 1, "lbfgs_success" : true, "pathfinder_success" : true, "x_center" : [ -1.50214 ], "logDetCholHk" : -2.90239, "L_approx" : [ [ -0.0548919 ] ], "Qk" : [  ], "alpha" : [ 1 ], "full" : true, "lbfgs_note" : ""},
"2" : {"iter" : 2, "unconstrained_parameters" : [ -1.46146 ], "grads" : [ -0.741069 ], "history_size" : 2, "lbfgs_success" : true, "pathfinder_success" : true, "x_center" : [ -1.09998 ], "logDetCholHk" : -0.358948, "L_approx" : [ [ 0.698411 ] ], "Qk" : [  ], "alpha" : [ 0.487778 ], "full" : true, "lbfgs_note" : ""},
"3" : {"iter" : 3, "unconstrained_parameters" : [ -1.08508 ], "grads" : [ 0.0305572 ], "history_size" : 3, "lbfgs_success" : true, "pathfinder_success" : true, "x_center" : [ -1.09861 ], "logDetCholHk" : -0.407202, "L_approx" : [ [ 0.66551 ] ], "Qk" : [  ], "alpha" : [ 0.442904 ], "full" : true, "lbfgs_note" : ""},
"4" : {"iter" : 4, "unconstrained_parameters" : [ -1.09822 ], "grads" : [ 0.000893818 ], "history_size" : 4, "lbfgs_success" : true, "pathfinder_success" : true, "x_center" : [ -1.09861 ], "logDetCholHk" : -0.405519, "L_approx" : [ [ 0.666631 ] ], "Qk" : [  ], "alpha" : [ 0.444397 ], "full" : true, "lbfgs_note" : ""}
}
mitzimorris commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

simplified. will try to remember this for next time.