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

More consistent instantiation from a .stan file in various languages #187

Closed
WardBrian opened this issue Nov 28, 2023 · 2 comments · Fixed by #188
Closed

More consistent instantiation from a .stan file in various languages #187

WardBrian opened this issue Nov 28, 2023 · 2 comments · Fixed by #188
Labels
code-cleanup julia python question Further information is requested

Comments

@WardBrian
Copy link
Collaborator

The changes I'm proposing in #172 do not expose a separate constructor for .stan files compared to .so files, but rather just branch on the extension of the file passed as the first argument.

So, in R, it would look like:

StanModel$new("foo_model.so", data="foo.data.json") # loads a model already compiled
StanModel$new("bar.stan", data="bar.data.json") # compiles, *then* loads

In Python this looks different for the two cases:

StanModel("foo_model.so", data="foo.data.json") # loads a model already compiled
StanModel.from_stan_file("bar.stan", data="bar.data.json") # compiles, *then* loads

In Julia it looks more like R, but is technically doing type dispatching rather than checking the .stan extension:

StanModel("foo_model.so", data="foo.data.json") # loads a model already compiled
StanModel(stan_file="bar.stan", data="bar.data.json") # compiles, *then* loads

I think the real outlier here is Python, which deserves to be unified IMO. We can leave the existing function, but just update some logic in the constructor to handle .stan files. Whether Julia should also be updated or left using this disbatching I am less sure.

@WardBrian WardBrian added question Further information is requested python julia code-cleanup labels Nov 28, 2023
@roualdes
Copy link
Owner

Agreed, Python should be unified so that StanModel("bar.stan", data = "foo.data.json") works. This also means adding a keyword argument named data, since in Python StanModel(...) currently has a keyword argument named model_data, different from R and Julia (which both use data). I like data better.

In Julia, I think allowing StanModel("foo_model.stan", ...) would be a good thing.

These changes would take some care to ensure backward compatibility, ya?

@WardBrian
Copy link
Collaborator Author

The easiest way to ensure backwards compatibility is just leave the existing functions but re-direct their logic to the new suped-up constructor. Renaming a keyword argument in Python is a bit more annoying to do in a backwards-compatible way, but doable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-cleanup julia python question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants