roualdes / bridgestan

BridgeStan provides efficient in-memory access through Python, Julia, and R to the methods of a Stan model.
https://roualdes.github.io/bridgestan
BSD 3-Clause "New" or "Revised" License
88 stars 12 forks source link

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

Closed WardBrian closed 10 months ago

WardBrian commented 10 months ago

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.

roualdes commented 10 months ago

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 commented 10 months ago

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.