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

save_metric_files: fix doc and error message, and add tests #1021

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jgabry
Copy link
Member

@jgabry jgabry commented Aug 7, 2024

Submission Checklist

  • Run unit tests
  • Declare copyright holder and agree to license (see below)

Summary

Fixes the doc to say that save_metric is FALSE by default and adds informative error message when save_metric_files() fails due to not having save the files. This was pointed out on the forum by @mippeqf: https://discourse.mc-stan.org/t/saving-metric-files-in-cmdstanr/36160.

@andrjohns with arguments like this we've been setting the default to NULL and just using the CmdStan default and in this case CmdStan defaults to not saving the metric files. But we could think about overriding the CmdStan default and set it to default to TRUE in CmdStanR. The downside is that this would be inconsistent with how we're treating most other arguments passed to CmdStan and it would save more files that most users never care about. The upside would be that it would save specific users the hassle of fitting a slow model and then realizing the default is save_metric=FALSE. I would lean towards just doing what I did in this PR and documenting that it's save_metric=FALSE by default, but I'm also open to changing the default.

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:

@jgabry jgabry requested review from andrjohns and rok-cesnovar and removed request for rok-cesnovar August 7, 2024 22:16
@jgabry jgabry changed the title save_metric_files: fix doc and add tests save_metric_files: fix doc and error message, and add tests Aug 7, 2024
@mippeqf
Copy link

mippeqf commented Aug 8, 2024

Thanks for fixing this so quickly @jgabry! I noticed that the fit$inv_metric() data is available even if the save_metric flag is set to FALSE. Doesn't that mean that the metric information is being saved regardless of whether the flag is set or not?

The only additional information that I found in the metric files beyond the matrix M was stepsize and metric_type, so most of what would be saved to the metric files is available regardless of whether the flag is set or not. Because the metric_type is likely also available either way, is the save_metric flag really responsible only for saving the stepsize?

@jgabry
Copy link
Member Author

jgabry commented Aug 8, 2024

Yeah it's a bit confusing. For a long time the only place the metric info was saved was in the output CSV file. This is where inv_metric() is getting that from. But recently CmdStan added the ability to write that info to a separate JSON file with idea, I think, of eventually removing the messy CSV parsing entirely. Currently cmdstanr supports both because both are in CmdStan, but I think at some point in the future we'll only have the info available from the JSON. In that case it may make sense to switch to defaulting to save the JSON files and to keep inv_metric() working by using the JSON behind the scenes. Sorry for the confusion!

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.

3 participants