cmdstanr icon indicating copy to clipboard operation
cmdstanr copied to clipboard

Expose new stan args

Open venpopov opened this issue 1 year ago • 11 comments

Submission Checklist

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

Summary

Expose the new arguments save_metric and save_cmdstan_config in Stan 2.34 as arguments in cmdstanr methods. Closes #931

  • the save_metric is now an argument to sample()
  • save_cmdstan_config is an argument for all sampling methods
  • both of these are set TRUE by default (as this is also the plan in CmdStanPy)
  • new methods metric_files(), config_files(), save_metric_files() and save_config_files() work similarly to the existing methods output_files() and save_output_files()

Now by default in addition to the output files for each chain, "*_metric.json" and "*_config.json" files are created alongside.


This PR only deals with the above. As I mentioned in #931, now that these are available, it would be good to read the metadata information directly from them at the end of sampling, instead of greping the metadata from the output files, to finalize the fit object faster. I can handle that next.

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): Vencislav Popov

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

  • Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
  • Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

venpopov avatar Mar 15 '24 18:03 venpopov

When using the new functions I noticed that cmdstan gives an error if running a model with adapt_engaged=FALSE and save_metric=TRUE. So I also added a test showing the failure and a check to turn off save-metric if adapt_engaged is FALSE

venpopov avatar Mar 17 '24 10:03 venpopov

Thank you! Will try to review this soon.

jgabry avatar Mar 19 '24 15:03 jgabry

As I mentioned in #931, now that these are available, it would be good to read the metadata information directly from them at the end of sampling, instead of greping the metadata from the output files, to finalize the fit object faster. I can handle that next.

This sounds great.

jgabry avatar Mar 19 '24 16:03 jgabry

Strange, the cmd check passed locally. Should I try to fix the issues or do you want to review the general approach first?

venpopov avatar Mar 19 '24 16:03 venpopov

I think the general approach is good, so if you want to take a look at the R cmd check failures now (at first glance it seems to be just a couple of small things) I think that's fine.

Possibly later today but maybe later this week I'll have a chance to make a few comments on various details in the PR, but overall looks good!

jgabry avatar Mar 19 '24 17:03 jgabry

Ok, I'll see to fixing the checks. As for the reading the metadata from the new files, I have noted a few questions to discuss first, but I'll probably have to post about it next week

venpopov avatar Mar 19 '24 17:03 venpopov

Ok, I'll see to fixing the checks.

Ok thanks. Otherwise this looks really good! I haven't had a chance to really test it yet, but I don't see any issues with the changes to the code.

As for the reading the metadata from the new files, I have noted a few questions to discuss first, but I'll probably have to post about it next week

Sounds good, thank you.

jgabry avatar Mar 19 '24 22:03 jgabry

When using the new functions I noticed that cmdstan gives an error if running a model with adapt_engaged=FALSE and save_metric=TRUE. So I also added a test showing the failure and a check to turn off save-metric if adapt_engaged is FALSE

that was the main culprit - the tests ran clean on the origin PR, and I did not run them again after this small change. I fixed and now everything runs clean on my machine, so hopefully the remote tests will pass as well.

venpopov avatar Mar 21 '24 13:03 venpopov

All the tests pass, except for WSL, which gives an error not being able to find the metric files. I'm not sure what to do about that - I don't have a WSL setup, so I don't know what the problem is or how to debug it.

venpopov avatar Mar 21 '24 15:03 venpopov

All the tests pass, except for WSL, which gives an error not being able to find the metric files. I'm not sure what to do about that - I don't have a WSL setup, so I don't know what the problem is or how to debug it.

Thanks!

@andrjohns would you be able take a peek at the WSL issue?

jgabry avatar Mar 25 '24 18:03 jgabry

Just bumping this up since it seems just that small issue with the WSL backend test is left. I could check the WSL tests in principle, but I would have to install a virtual windows machine and set up everything just for this. It would be great if someone who already has access to WSL backend could check what the problem is ;)

venpopov avatar Apr 02 '24 17:04 venpopov

Hoping @andrjohns can take a look at this since I don't know much about WSL. @andrjohns if you're too busy at the moment let me know and I can try to find someone else who knows WSL to take a look.

jgabry avatar Apr 12 '24 16:04 jgabry

Sorry for the delay, I'll have time to fix up the WSL failures and review on Friday

andrjohns avatar Apr 16 '24 19:04 andrjohns

@venpopov I've gone ahead implemented my requests, let me know if you have any other changes or updates you want to make, otherwise I'll merge this

Thanks again for putting this together!

andrjohns avatar May 04 '24 12:05 andrjohns

Thanks! I've been on vacation without a computer and was planning to get to it next week. But if you are happy with the current state, feel free to merge 👍

venpopov avatar May 04 '24 17:05 venpopov