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

Convert rstan to a suggests to avoid windows installation issues #405

Closed
wants to merge 5 commits into from

Conversation

gowerc
Copy link
Collaborator

@gowerc gowerc commented Jan 16, 2024

Closes #400

Hey @nociale / @wolbersm , Would you mind testing this out to see if it works for you. Essentially I've removed rstan from the imports and moved it to the suggests. This way rstan isn't installed by default with the package however the package errors if you try to use method_bayes and rstan isn't installed.

The model is now compiled on the fly. Storing the compiled model is non-trivial though, for ease of programming I've currently just left it to the rstan default behaviour which is to store it in the session directory; this means the model will need to be recompiled once per session. If this is considered too much I could try writing it to the users home directory (or offering an option for the user to specify where to cache the model?) but my gut feeling was this is a suitable compromise.

@gowerc gowerc requested a review from nociale January 16, 2024 17:33
@gowerc gowerc changed the title Convert rstan to a suggests to avoid unnecessary compilation Convert rstan to a suggests to avoid windows installation issues Jan 16, 2024
@gowerc
Copy link
Collaborator Author

gowerc commented Jan 19, 2024

Hmm with regards to the model re-compiling I am getting inconsistent behaviour; sometimes it only needs to be compiled once per session other times it needs to be re-compiled one time per usage. It is not clear to me how rstan is determining when the model does / doesn't need to be re-compiled. I've asked for help on the stan forums and done a lot of searching but there doesn't appear to be any clear answer without digging into the rstan source code.

My gut feeling is that this is fine to accept for now but we should aim to convert rstan to cmdstanr when it eventually makes it to CRAN as this has much better handling of compiled models in my experiance on other packages.

@gowerc
Copy link
Collaborator Author

gowerc commented Oct 4, 2024

Going to close this and re-do as I've forgotten what changes were made any why

@gowerc gowerc closed this Oct 4, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider making Stan a suggested package
1 participant