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

Can/should StanRNG API be more consistent across the interfaces? #120

Closed roualdes closed 1 year ago

roualdes commented 1 year ago

As I read our doc, Python and R detail creation of a StanRNG object from methods on their respective StanModel classes, e.g. StanModel.new_rng(seed) and StanModel$new_rng(seed). Neither detail direct calls to StanRNG.

Julia on the other hand details direct calls to StanRNG, but does not have a new_rng(sm::StanModel, seed) function.

I could really go either way on adding new_rng(sm::StanModel, seed) to Julia. On the one hand, it would make things more consistent with the other interfaces, but it would literally just be another name for what StanRNG already does, same arguments and all.

On the other hand, I think it makes sense to doc StanRNG constructors for R and Python.

What do you think?

WardBrian commented 1 year ago

but it would literally just be another name for what StanRNG already does, same arguments and all.

Yeah, I went with this different style in Julia due to this and the more general fact that Julia doesn't let you directly associate methods with a struct the way Python and R's classes do. I think we could literally write new_rng = StanRNG in the Julia file and it would be equivalent.

In R and Python, the constructor of the StanRNG is internal/not documented online because it is written such that it takes in the handle to the library directly, rather than a handle to the model.

We sweep this distinction under the rug for the user, but creating an RNG doesn't really require anything from the model instance, but it does require accessing the library the model came from.

roualdes commented 1 year ago

In R and Python, the constructor of the StanRNG is internal/not documented online because it is written such that it takes in the handle to the library directly, rather than a handle to the model.

Right. Would it be better to have

class StanRNG:
    def __init__(self, sm: StanModel, seed: int) -> None:
        self.stanlib = sm.stanlib
        ...

and equivalent in R, since this is effectively what happens in Julia?

WardBrian commented 1 year ago

I think I'd rather keep those as they were and add the new_rng alias to the Julia interface if method-for-method parity is the goal

sethaxen commented 1 year ago

To avoid potential confusion, I suggest not using an alias and instead defining

new_rng(sm::StanModel, seed) = StanRNG(sm, seed)

This is similar to how Random defines default_rng([tid]): https://github.com/JuliaLang/julia/blob/e204e200df8f57e516920c056b6ec2eb37d34079/stdlib/Random/src/RNGs.jl#L346-L347