-
Notifications
You must be signed in to change notification settings - Fork 428
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
More general Exp #969
Comments
I'm not sure I understand what the problem is with constructing a new one. Is this about performance or convenience? If you need this edge case behavior, couldn't you implement your own distribution on top of |
|
Thank you for the quick answer!! Sorry for not being clear. I will try again. General parametrization
Yes, from my point of view it is (Wikipedia does not think the same). Implications
The problem is that the implementation changes. An exponential with
I am okay using my own wrapper, I just wanted to ask if you had better ideas. Setter method
There is no problem with constructing a new one, a method |
I was thinking optimise for the common case, waste a sample and multiply by INF in the zero case. Is that not acceptable?
When designing an API for general usage, it is always a good idea to minimise complexity: avoid methods you don't need. And I don't think a setter adds enough. |
Good!! I didn't think about it. In fact, there is no need to waste a sample! This is the current implementation. #[derive(Clone, Copy, Debug)]
pub struct Exp<N> {
/// `lambda` stored as `1/lambda`, since this is what we scale by.
lambda_inverse: N
}
/// Error type returned from `Exp::new`.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum Error {
/// `lambda <= 0` or `nan`.
LambdaTooSmall,
}
impl<N: Float> Exp<N>
where Exp1: Distribution<N>
{
/// Construct a new `Exp` with the given shape parameter
/// `lambda`.
#[inline]
pub fn new(lambda: N) -> Result<Exp<N>, Error> {
if !(lambda > N::from(0.0)) {
return Err(Error::LambdaTooSmall);
}
Ok(Exp { lambda_inverse: N::from(1.0) / lambda })
}
}
impl<N: Float> Distribution<N> for Exp<N>
where Exp1: Distribution<N>
{
fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> N {
rng.sample(Exp1) * self.lambda_inverse
}
} One could change the constructor method (or create another constructor to not have breaking change) like this. pub fn new(lambda: N) -> Result<Exp<N>, Error> {
if lambda > N::from(0.0) {
return Ok(Exp { lambda_inverse: N::from(1.0) / lambda });
} else if lambda < N::from(0.0) {
return Err(Error::LambdaTooSmall);
} else {
Ok(Exp { lambda_inverse: N::infinity() })
}
}
} Consequences
|
@rasa200 that's roughly what I was thinking, yes. It does still sample @vks any thoughts on whether we should allow zero-rate in the |
I think either way is fine as long as it is documented. |
Sure! It is good! |
Okay then, I guess the only code change we need is to @rasa200 would you like to make a PR? |
@dhardy Happy to make a PR :D It will be my first ever. One question: To use Another possibility is using
Historical note: In rand, version 0.7.3, rand::distributions::Exp can be created only with |
You shouldn't need to. I just tested: /// Construct a new `Exp` with the given shape parameter
/// `lambda`.
#[inline]
pub fn new(lambda: N) -> Result<Exp<N>, Error> {
if !(lambda >= N::from(0.0)) {
return Err(Error::LambdaTooSmall);
}
Ok(Exp {
lambda_inverse: N::from(1.0) / lambda,
})
}
}
...
#[cfg(test)]
mod test {
use super::*;
#[test]
fn test_zero() {
let d = Exp::new(0.0).unwrap();
assert_eq!(d.sample(&mut crate::test::rng(21)), std::f64::INFINITY);
} Yes, supporting both float types is a recent change. |
True!! My bad, I should have tried first. |
There is a PR :D I had a bit of travel with the documentation (see PR), because of the possibility of a custom implementation of |
Background
What is your motivation?
I am implementing stochastic processes, continuous markov chains in particular, and I need an Exponential clock. I wanted to use rand_distr::Exp as an internal field, but I have the following two problems:
set_lambda
to change it, instead of having to construct a new one.What type of application is this?
Numerical simulation
Feature request
Generalize the construction of
Exp
to include the case oflambda = 0.0
.Also, implement a "setter" method to change the
lambda
in an already createdExp
.The text was updated successfully, but these errors were encountered: