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 expose_stan_functions under Stanc 2.31 #1050

Merged
merged 9 commits into from
Mar 16, 2023

Conversation

andrjohns
Copy link
Contributor

@andrjohns andrjohns commented Mar 16, 2023

Part of the expose_stan_functions_hacks code was replacing = nullptr with = 0, intended for the pstream__ = nullptr default argument. However this causes a compilation failure under 2.31, since the generated code now includes default template parameters with nullptr which are then replaced.

This PR updates the regex to only replace the nullptr if it's followed by a parenthesis - as the function definitions will always generate the pstream__ = nullptr as the last argument. I've also added a basic test.

This will also need to be backported to 2.26

@andrjohns
Copy link
Contributor Author

I've also verified that this fixes the test failure for the prophet package under 2.31

@@ -31,7 +31,7 @@ expose_stan_functions_hacks <- function(code, includes = NULL) {
code <- gsub("stan::math::accumulator<double>& lp_accum__, std::ostream* pstream__ = nullptr){",
"std::ostream* pstream__ = nullptr){\nstan::math::accumulator<double> lp_accum__;",
code, fixed = TRUE)
code <- gsub("= nullptr", "= 0", code, fixed = TRUE)
code <- gsub("= nullptr)", "= 0)", code, fixed = TRUE)
Copy link
Member

@hsbadr hsbadr Mar 16, 2023

Choose a reason for hiding this comment

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

Is there other patterns that we need to support here, such as "nullptr,", ",nullptr", or ", nullptr"?

Copy link
Contributor Author

@andrjohns andrjohns Mar 16, 2023

Choose a reason for hiding this comment

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

I would have said no, but the test failure under ubuntu is making me question that. I'll investigate and update the PR

@hsbadr
Copy link
Member

hsbadr commented Mar 16, 2023

Does this fix #954?

@andrjohns
Copy link
Contributor Author

It's bizarre, I can't replicate the test failures locally under MacOS, Ubuntu, or Windows

andrjohns and others added 2 commits March 16, 2023 12:31
To reduce the artifact size if check is failed.
@andrjohns
Copy link
Contributor Author

Narrowed it down a bit more. The test will fail when run through rcmdcheck() but not when run through test_file()

@andrjohns
Copy link
Contributor Author

Ah, it looks like sourceCpp-based functions aren't compatible with testthat, since it modifies include paths: https://stat.ethz.ch/pipermail/r-package-devel/2017q3/001951.html

May have to just remove the test entirely (after testing locally)

@hsbadr
Copy link
Member

hsbadr commented Mar 16, 2023

it looks like sourceCpp-based functions aren't compatible with testthat,

I see. Could probably use Catch: https://testthat.r-lib.org/reference/use_catch.html.

Anyway, it works on my local machine.

Copy link
Member

@hsbadr hsbadr left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants