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

WIP Rust interface for bridgestan #102

Conversation

aseyboldt
Copy link
Contributor

This is based on #88 and @WardBrian's branch.
Depends on #98 and will need to be rebased when that is merged.

It is still not decided if this should live in nutpie or in bridgestan, either would be fine for me.

I made a couple of changes compared to Brians branch:

  • The shared library is stored as a Borrow<StanLibrary>, so that client code can decide what kind of reference should be used to store the Library (ie Rc, Arc, &lib or direct ownership). That avoids a whole range of headache when using this in multithreaded code.
  • open_library checks that the version of the library has the same minor and major version as the rust library.
  • Model is now Send and Sync
  • STAN_THREADS=true is required
  • Some more functions are wrapped
  • If there is non-utf8 in an error message, it will now still return the message, but replace the invalid characters by placeholders.

Some open questions:

I'm not 100% sure about the handling of string/bytes data in the interface. It might be easier to return &CStr in a couple of places, and leave worrying about the encoding to users.

param_unconstrain: What lengths are allowed for theta? I'm assuming that include_tp, inclued_gq are both set to false right now...

@aseyboldt aseyboldt mentioned this pull request Apr 5, 2023
@WardBrian WardBrian changed the base branch from main to feature/gq-thread-safety April 5, 2023 23:53
@WardBrian
Copy link
Collaborator

I've changed the base branch just to make viewing the diff easier, it would obviously still be merged into main ultimately

@WardBrian
Copy link
Collaborator

TIL about Borrow<T>. Now I need to learn why one would prefer that over AsRef<T>.

For string encoding, I think you need to be in a really weird edge case to end up with a mangled error string. All of the "normal" Stan error messages should be plain ASCII, and I think any libraries we use in C++ would be the same. I'm personally happy to do the encoding for now and re-assess if we ever find a case it doesn't work in.

@WardBrian
Copy link
Collaborator

param_unconstrain: What lengths are allowed for theta? I'm assuming that include_tp, inclued_gq are both set to false right now

I'm hoping that the internals on this function change soon, but for both the current implementation and the one I'm hoping to use at some point in the future the requirement is that the length is at least bs_param_num(false,false), but longer is fine (we just end up ignoring unused entries in the buffer)

@aseyboldt
Copy link
Contributor Author

TIL about Borrow. Now I need to learn why one would prefer that over AsRef.

The reason I wanted Borrow is this impl (which isn't available for AsRef):
https://doc.rust-lang.org/std/borrow/trait.Borrow.html#impl-Borrow%3CT%3E-for-T

This makes it possible to pass in the library directly (and not a reference at all), so that the model owns the library.

@WardBrian WardBrian deleted the branch roualdes:feature/gq-thread-safety April 14, 2023 16:31
@WardBrian WardBrian closed this Apr 14, 2023
@aseyboldt
Copy link
Contributor Author

@WardBrian I guess this got closed because the branch was deleted?

@WardBrian
Copy link
Collaborator

Oh, yeah, forgot that would happen!

this will need some rebasing anyway because of things like the constructor function names changing

@aseyboldt
Copy link
Contributor Author

I just did, I can just put it into a new PR or someone reopens this one (I can't)

@WardBrian
Copy link
Collaborator

I also cannot reopen this. I guess there’s no way to change the base after the base has been deleted

@aseyboldt aseyboldt mentioned this pull request Apr 14, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants