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

Support for USER_HEADER in build system #233

Merged
merged 4 commits into from
May 7, 2024
Merged

Conversation

WardBrian
Copy link
Collaborator

This ports the portion of the CmdStan makefile that allows for easier inclusion of external C++ functions:
https://mc-stan.org/docs/cmdstan-guide/external_code.html

Essentially, if the --allow-undefined option is passed to stanc3, we then look for a header file to be included during compilation. By default this is user_header.hpp in the same folder as the Stan model, but it can be changed by setting USER_HEADER.

I'm not sure where is the best place to slot in documentation for this.

@WardBrian WardBrian added enhancement New feature or request cpp labels May 3, 2024
@WardBrian WardBrian requested a review from roualdes May 3, 2024 20:52
@roualdes
Copy link
Owner

roualdes commented May 3, 2024

Great addition. Thanks.

I suppose the Customizing BridgeStan part of the doc? With a link to the CmdStan User's Guide.

Any idea why the Rust tests failed?

@WardBrian
Copy link
Collaborator Author

Rust tests try to load all the modules from the folder, so I think we just need to exclude this the same way we exclude syntax_error.

Customizing BridgeStan is probably the best section, though it's more per-model that most of the other recommendations there

@roualdes
Copy link
Owner

roualdes commented May 6, 2024

Let me know when you are ready for me to look this over again. Thanks.

@WardBrian
Copy link
Collaborator Author

@roualdes I think it's ready for another look!

Copy link
Owner

@roualdes roualdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick question, but overall looks good to me. A nice quality of life feature to add to BridgeStan. Thanks.


Namely, you can declare a function in your Stan model and then define it in a separate C++ file.
This requires passing the ``--allow-undefined`` flag to the Stan compiler when building your model.
The :makevar:`USER_HEADER` variable must point to the C++ file containing the function definition.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we say explicitly that it should be a C++ header file? We only reference it being a .hpp file in the makevar variable name and the example file name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It technically does not need to be a header (or even a .hpp, in theory a .txt would also work, since the compiler is just pasting it in), as long as it's code that can be included into the model's translation unit it would be fine.

Personally I think the name USER_HEADER is evocative enough

@WardBrian WardBrian merged commit fb734ea into main May 7, 2024
19 checks passed
@WardBrian WardBrian deleted the feat/external-cpp branch May 7, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants