-
Notifications
You must be signed in to change notification settings - Fork 21
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
Support and test in-place sampling #176
Conversation
Codecov Report
@@ Coverage Diff @@
## master #176 +/- ##
==========================================
+ Coverage 99.23% 99.24% +0.01%
==========================================
Files 9 9
Lines 260 266 +6
==========================================
+ Hits 258 264 +6
Misses 2 2
Continue to review full report at Codecov.
|
rand | ||
rand! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we actually replace this by Distributions._rand!
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this would be reasonable if we want to follow the conventions in Distributions. I am just not sure if we want to do this, it feels weird if an internal method such as _rand!
is part of the API (of course, this is a problem of Distributions but since we already define rand
separately maybe we want to not follow it here either).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want to follow the conventions in Distributions
That seems like something we want to do no? Maybe we can add a reference to their API docs for better understanding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if we really want to follow their conventions we would/should not have to implement rand
. However, I really think we should not do this since it is quite likely to lead to incorrect types (I checked and tests fail since the output type is incorrect for Float32
- of course, one could work around this by defining eltype(::FiniteGP)
but I think the easier solution is to only perform in-place sampling if requested 🤷). So we do not follow the abstractions in Distributions even if we tell users to implement Distributions._rand!
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other reason to like having an explicit implementation for rand
is AD, since our AD tools really don't like mutation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also happy with this.
Currently something like
throws an error since
Distributions._rand!
is not defined forFiniteGP
s (this is the method that Distributions falls back to when one callsrand!
for multivariate distributions and that one is supposed to define for new multivariate distributions: https://juliastats.org/Distributions.jl/latest/extends/#Multivariate-Sampler).This PR adds a definition of
Distributions._rand!
forFiniteGP
s. Distributions also falls back to it when callingrand
but I think often this is a bad idea (one has to use type heuristics that fail sometimes - e.g. tests fail if one removes the custom definitions in AbstractGPs) and so I did not remove or modify the existing definitions ofrand
. It might still be useful to defineDistributions._rand!
instead ofRandom.rand!
since then e.g. Distributions already checks if the size of the input is correct and one gets support for arrays of vectors for free (see https://github.com/JuliaStats/Distributions.jl/blob/a0cb0969d755872586ec79e985503f03beae9d08/src/multivariates.jl#L47-L66).