rust-random / rand

A Rust library for random number generation.

https://crates.io/crates/rand

Other

1.67k stars 432 forks source link

Add rand_distr::TruncatedNormal #1523

Closed ongchi closed 4 days ago

ongchi commented 2 weeks ago

[x] Added a CHANGELOG.md entry

Summary

Add truncated normal distribution to rand_distr, allowing users to specify the mean, standard deviation, left bound, and right bound of the distribution.

Motivation

As discussion in #1189. This would be use in numerical simulations, statistics, games, etc.

Details

This implementation is ported from scipy.stats.truncnorm, which relies on Cephes Mathematical Library. An extra dependency is added of the Rust implementation of the Cephes library (spec_math).

dhardy commented 2 weeks ago

So this is a form of inverse CDF? Quick points:

The code presented would be best in a separate file. (This may not be the case with just using Normal with rejection.)
There may be licencing concerns from BSD to our licence.
How does this approach compare in performance to just using rejection sampling? This would depend on the bounds of course; what bounds may be likely?

benjamin-lieser commented 2 weeks ago

I would say the value of a truncated distribution implementation comes from it working also on the tails of the distribution where rejection sampling would be infeasible.

ongchi commented 1 week ago

Yes, the implementation is based on the inverse CDF method. I can move the code into a separate file, as suggested. Would adding a BSD license header to the file be acceptable for this project?

And I just add a simple benchmark.

The x-axis represents the relative cumulative frequency between the left and right bounds of the normal distribution.

by_rej by_ppf

The benchmark shows that the performance of the rejection sampling method significantly degrades when the relative cumulative frequency of the bounds is below 5%. In contrast, the inverse CDF (PPF) method maintains constant time performance regardless of the bounds.

dhardy commented 1 week ago

Would adding a BSD license header to the file be acceptable for this project?

Unfortunately not. We distribute the project under the users' choice of MIT or Apache 2.0, which means that all contributions must satisfy both of these licences. It seems that scipy is distributed under the 3-clause BSD license. I'm not an expert on the topic, but from what I understand this licence is not compatible with distribution under the MIT licence.

(Note that we can add attributions as a comment in the source, so I don't believe that part is a problem at least.)

Hence, unfortunately, I don't believe we can accept this — unless you can obtain a statement of permission from the original authors, or remove all code which was ported from scipy.

ongchi commented 4 days ago

Thank you for your comment. I will close this PR for now and take some time to decide what to do on the next steps.