Distributions documentation

[dhardy]

Pulled commits relevant to the distributions module out of #423. @pitdicker you should be able to push directly to this branch if you wish.

I'll go over the Uniform doc myself...

[pitdicker]

You made a branch in the nursery, that is an option too 😄.
And good idea to split it out, best to land in parts.

Do you want the rename of UniformImpl in this PR or another?

[dhardy]

What do you think about error-handling — I've been wondering about making UniformImpl::new etc. return Result<Self, UniformError> and unwrapping in Uniform — but allowing users to use the result-versions if preferred.

[dhardy]

I put together some code showing how error-handling could work, but I'm not convinced it's a good idea — extra complexity for what seems little gain; plus I realise our ErrorKind was written purely with RNGs in mind!

[pitdicker]

Good job on the uniform module!

I put together some code showing how error-handling could work, but I'm not convinced it's a good idea — extra complexity for what seems little gain; plus I realise our ErrorKind was written purely with RNGs in mind!

Personally I would not really want to see error handling here, in my opinion it is a programming error if the lower bound is greater than the upper bound of the range.

I did have a thought about where to put the asserts this morning. I was trying to implement the Uniform distribution for Wrapping<T>. I think for that type, it makes sense to have the range wrap around if low > high. But that is only possible if the check is in the UniformSampler implementation, not in the Uniform wrapper.

[dhardy]

Moving the asserts there inherently makes more sense — except that sometimes new is defined in terms of new_inclusive and sometimes the other way around, so this would force an assert to run an extra time. In the case of UniformFloat, both functions must have the same assert when done this way, unless the implementation is repeated/moved. Actually this is one of the reasons I don't like my error refactor code, so if we must move the asserts we can consider revisiting that.

But that's another issue. If you're happy with this PR then merge.

[vks]

Inlining might remove double asserts.

…

On Tue, May 8, 2018, 18:39 Paul Dicker ***@***.***> wrote: Merged #433 <#433>. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#433 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACCtGKXtAu3jJzxbSrb5pPuTUVhw7p_ks5twcoygaJpZM4T2YyG> .