Allow user to supply a c++ header file #464
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #464 +/- ##
===========================================
+ Coverage 77.95% 78.06% +0.10%
===========================================
Files 30 30
Lines 9168 9231 +63
===========================================
+ Hits 7147 7206 +59
- Misses 2021 2025 +4
Continue to review full report at Codecov.
|
|
@mitzimorris this is now failing with 2.28 and wasn’t before, need to investigate tomorrow but very odd |
|
merges #461, this should clear up spurious failures |
There was a problem hiding this comment.
looks good and appreciate the global cleanup.
question for @rok-cesnovar and @jgabry - add similar functionality to CmdStanR?
CmdStanR supports this already via the cpp_options, stanc_options: library(cmdstanr)
stan_file <- "bernoulli_external.stan"
mod <- cmdstan_model(
stan_file,
cpp_options = list(USER_HEADER="/home/rok/Desktop/testing/external/make_odds.hpp"),
stanc_options = list("allow-undefined")
)
data_list <- list(N = 10, y = c(0,1,0,0,0,0,0,0,0,1))
fit <- mod$sample(data = data_list)The only "problem" we currently have is one needs to specify the full path to the .hpp file. And we need to make a vignette. Users keep asking on how to do this. Though the namespace thing in the external file is annoying -> stan-dev/stanc3#712 |
|
@WardBrian - should CmdStanPy do the same? do we need to introduce add'l argument
|
|
Having it separate is nice because it lets us allow relative paths and do some checking on those paths from within python. I didn't even think to try it the other way, but if we want we can just take the example notebook I wrote and adapt it to that style |
that seems like a good enough reason to me. happy to add this argument. |
|
I checked, and it does indeed work in the example I wrote up to replace model_external.compile(user_header='make_odds.hpp')with model_external.compile(cpp_options={'USER_HEADER':'/home/brian/Dev/py/cmdstanpy/docsrc/examples/make_odds.hpp'}, stanc_options={'allow_undefined':True})While I do think it is a good idea to have this as a separate argument, if only so we can do some path resolution, I'm going to rewrite it slightly so it uses |
|
update: nevermind, my local cleanup script clobbered the .hpp file. |
| "We can provide a definition in a C++ header file by using the `user_header` argument to either the CmdStanModel constructor or the `compile` method. \n", | ||
| "\n", | ||
| "This will enables the `allow_undefined` flag automatically." | ||
| ] | ||
| }, | ||
| { | ||
| "cell_type": "code", | ||
| "execution_count": null, | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "model_external.compile(user_header='make_odds.hpp')" |
There was a problem hiding this comment.
I'm not sure what you mean. The user interface is still to use model_header, this just gets turned into part of the CPP options during validation
There was a problem hiding this comment.
you're right - user error on my part - testing in wrong environment.
Submission Checklist
Summary
Adds the ability to specify a .hpp file for external function definitions. Closes #463.
This requires storing some .hpp files in the test data, which overcomplicated the removal routine. I've switched to a global fixture which uses
git clean -Xf data/to remove all files in the git ignore in the test/data folder. This is only run once per session at the moment, at the end of all testing.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):
Simons Foundation
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: