Closed hyiltiz closed 9 months ago
I've been informed that this PR is sort of a "reboot" of this earlier one due to technical issues :)
Seems to be working here -- at least the tests pass :)
Some minor points:
Most (all?) of the existing docstrings don't contain example invocations. I think sample invocations are helpful, but placing such things in tests is an ok alternative for spork. I'd go for removing the examples from the docstrings of rand-gaussian
and sample-n
. The current PR already contains some tests that reflect some of the examples from the docstrings so perhaps additional ones aren't necessary. I'm not opposed though :)
There appear to be some commented code bits remaining:
(let [rho (math/sqrt (* -2 (math/log q)))
theta (* 2 math/pi p)
# _box (* rho (math/cos theta))
_muller (* rho (math/sin theta))
# box (scale _box)
muller (scale _muller)]
# (yield box)
muller))
Not sure what others will say, but if these are to stay, may be it makes sense to have some commentary explaining their presence. I can see the value of mentioning that sort of half of what one might have expected is being discarded for future readers / maintainers [1]. Not a fan of the yield
form even commented here.
[1] Wasn't clear on the choice to retain muller
over box
-- I think a previous version went with the reverse choice.
Updated w.r.t. feedback.
I'm a bit fuzzy on this, but did we not conclude that using yield
would lead to undesirable places wrt the function being usable in certain types of scenarios?
If so, I think leaving this comment is on the misleading side and potentially asking for trouble down the line.
Thanks for the update.
LGTM.
Ready to merge and close it out.
This adds a Gaussian sampler that exposes a similar interface to
spork/rand-uniform
, that is:(rand-gaussian)
for a single standard Gaussian sample(rand-gaussian 5 0.1)
for a single Gaussian sample with specified parametersN
samples with thesample-n
function, using(sample-n |(rand-gaussian 5 0.1) 100)