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

Julia interface improvements #11

Closed
roualdes opened this issue Sep 8, 2022 · 7 comments · Fixed by #12
Closed

Julia interface improvements #11

roualdes opened this issue Sep 8, 2022 · 7 comments · Fixed by #12
Labels

Comments

@roualdes
Copy link
Owner

roualdes commented Sep 8, 2022

Here's a list of Julia interface items I'd like to tackle one day.

  • adopt more idiomatic package name spelling, BridgeStan instead of Bridgestan
  • export K means nothing since K is never defined
  • its common Julia practice to use f! when there's an out argument; add log_density, log_density_gradient (no exclamation points) which assign to new memory (instead of using an out argument)
  • can drop the ::Vector{Float64} in most use cases of zeros(D); Float64 by default
  • not all defined functions are exported
  • double check tests for completeness
@WardBrian
Copy link
Collaborator

I definitely copied the export list without doing anything to it, my bad!

Re:out parameters, I believe the functions are currently implemented as you describe (new memory each time) so we should add the ! versions

for tests we should probably just recreate the Python tests in all 3 languages eventually, those are the most complete

@roualdes
Copy link
Owner Author

roualdes commented Sep 8, 2022

No blame here.

Yep, I see that now. You're right. We should the ! versions.

Agreed on the tests.

@WardBrian
Copy link
Collaborator

Would you like me to take a crack at some of these, or would you prefer to do it?

@roualdes
Copy link
Owner Author

roualdes commented Sep 9, 2022

Yes, please do. I phrased to to do list in the first person so as not to tell anybody what to do. I'd appreciate you giving it a go. Thanks.

@roualdes
Copy link
Owner Author

roualdes commented Sep 9, 2022

@WardBrian, what do you think about adding

export BS
const BS = BridgeStan

to the BridgeStan module and dropping all other exports? The effect would be

using BridgeStan

then enables BS.log_density() and BS.log_density_gradient(), ... .

This is what they do in AbstractDifferentiation.jl.

It's like a Julia version of Python's import bridgestan as bs.

To me the pithiness is nice, but I could see it also being not obvious.

@WardBrian
Copy link
Collaborator

A user could still do “const BS = BridgeStan” on their own, right?

@roualdes
Copy link
Owner Author

roualdes commented Sep 9, 2022

That's fair. It is probably better just to remind users that const BS = BridgeStan is possible, in say the example.jl, rather than force it on them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants