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

Add functionality to build models inside R #172

Merged
merged 3 commits into from
Nov 29, 2023
Merged

Add functionality to build models inside R #172

merged 3 commits into from
Nov 29, 2023

Conversation

WardBrian
Copy link
Collaborator

@WardBrian WardBrian commented Sep 26, 2023

This addresses part of #100 and is more or less a straight port of the Python functionality.

I'm not at all a confident R programmer, so I'm going to ask if @jgabry or @rok-cesnovar could kindly spare some time to look this over.

@WardBrian WardBrian added enhancement New feature or request help wanted Extra attention is needed R-lang labels Sep 26, 2023
@WardBrian WardBrian force-pushed the R/build-in-R branch 4 times, most recently from fc50988 to 1ac29e6 Compare November 8, 2023 21:42
@WardBrian WardBrian marked this pull request as ready for review November 9, 2023 19:42
@WardBrian
Copy link
Collaborator Author

I think this is ready now, but I'm still seeking some volunteer review from some of the Stan R programmers who will know more than me: @rok-cesnovar @jgabry @bgoodri

If any of you have some spare cycles, it isn't all that much code. I'd be happy to hop on a call and talk through it as well. Appreciate it if you're able!

@rok-cesnovar
Copy link
Contributor

If you can walk me through this, would be happy to review it. We can schedule on Slack.

Copy link
Contributor

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

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

Brian walked me through the changes and it all makes sense to me.

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.

Thanks to you both, Brian and Rok. There's some nits in here, but overall this looks great. Much appreciated.

R/R/compile.R Outdated Show resolved Hide resolved
R/R/compile.R Outdated Show resolved Hide resolved
return(output)
}

windows_path_setup <- function() {
Copy link
Owner

Choose a reason for hiding this comment

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

I missed it. Where is this function used?

Copy link
Collaborator Author

@WardBrian WardBrian Nov 29, 2023

Choose a reason for hiding this comment

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

It's called in the constructor before we load the shared object (same as Python, looks like Julia calls it at startup which is not ideal and I will take a look at with #187)

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.

Looks good to me. Thanks much.

@WardBrian WardBrian merged commit 41b66b7 into main Nov 29, 2023
19 checks passed
@WardBrian WardBrian deleted the R/build-in-R branch November 29, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed R-lang
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants