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

Fix model$format and model$check_syntax for compiled models with include-paths #775

Merged
merged 2 commits into from Jun 23, 2023

Conversation

adrian-lison
Copy link
Contributor

Submission Checklist

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

Summary

This PR fixes #774 by using self$include_paths() instead of private$precompile_include_paths_ in the methods check_syntax and format. This is because the latter only holds the include_paths if the model has not been compiled yet. As a result, it is now possible to call these methods on compiled models with include statements without getting a syntax error from stanc. For format, this has currently not been possible at all, for check_syntax, you always had to use the include_paths argument of the function even if the same, default include_paths were used.

The PR also adds tests for include_paths in check_syntax and format on compiled models, but I am not sure if these are desired as they require compilation of the example model during testing.

This is my first PR for cmdstanr, so apologies in case I violated any conventions.

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):
Adrian Lison

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

@rok-cesnovar
Copy link
Member

Thanks so much @adrian-lison! This looks great.

@andrjohns
Copy link
Collaborator

The json failures in the CI here are due to the jsonlite package (opened issue here).

As only the json tests failed and all other passed, I'll merge this

@andrjohns andrjohns merged commit b78b843 into stan-dev:master Jun 23, 2023
0 of 11 checks passed
@jgabry
Copy link
Member

jgabry commented Jun 29, 2023 via email

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.

model$format and model$check_syntax fail to account for include-paths on compiled models
4 participants