Created an attachment (id=135) [details]
Minor patch

(From update of attachment 135 [details])
This patch only addresses the INFINITE_VALUE issue.  The bounds issue needs
feedback as its more about expectations & usability, so it has been left alone
here.

(In reply to comment #2)
> (From update of attachment 135 [details] [edit])
> This patch only addresses the INFINITE_VALUE issue.  The bounds issue needs
> feedback as its more about expectations & usability, so it has been left alone
> here.
> 

The bounds setting API is probably non optimal.  If anything, the range should
extend [mean-bound,mean+bound], that way the distribution mean and mode are
preserved.  Asymmetric bounds causes the mean of the bounded distribution to be
different from what you specified. 

Created an attachment (id=156) [details]
Proposed (untested fix)

Subsumes Joe's patch.  Does symmetrical bounds, I'm just doing some quick
histogram plots to verify.

Created an attachment (id=157) [details]
test case for above patch

This is a test case which generates both unbounded and bounded versions of a
Normal(10,2).  Histogram plots verify that it works at bounding the
distribution symmetrically about the mean.  The tail of the bounded
distribution is skewed however:
http://www.prism.gatech.edu/~gtg037s/files/normal-variable.eps

This is due to the way the code is forcing values outside of the range to be on
the range boundary.  Ideally, the code would drop values outside of the range
and "try again" so that the boundary didn't have this behavrior.

Honestly though, this API was never intended to do this.  I conceived that the
bound would be set somewhere like 5 standard deviations away, so that this tail
effect is minimized.  What do people think?

(In reply to comment #5)
> Created an attachment (id=157) [edit] [details]
> test case for above patch
> 
> This is a test case which generates both unbounded and bounded versions of a
> Normal(10,2).  Histogram plots verify that it works at bounding the
> distribution symmetrically about the mean.  The tail of the bounded
> distribution is skewed however:
> http://www.prism.gatech.edu/~gtg037s/files/normal-variable.eps
> 
> This is due to the way the code is forcing values outside of the range to be on
> the range boundary.  Ideally, the code would drop values outside of the range
> and "try again" so that the boundary didn't have this behavrior.
> 
> Honestly though, this API was never intended to do this.  I conceived that the
> bound would be set somewhere like 5 standard deviations away, so that this tail
> effect is minimized.  What do people think?

From the perspective of the user, I think that forcing values outside of the
range to be on the edge of the range is actively harmful. Can't you just drop
values outside of the range and retry (from within the random variable code)
and return a value when it is within the range only ?

Created an attachment (id=160) [details]
updated to throw away values outside of range and "try again"

This does what I described in my last comment.  It throws away values outside
of the range and keeps trying until it can return a valid number:
http://www.prism.gatech.edu/~gtg037s/files/normal-variable.eps

Same test case, same normal 10,2 distribution.  I would push, but this needs
some feedback from our new maintainer Michele Weigle.

(In reply to comment #7)
> Created an attachment (id=160) [edit] [details]
> updated to throw away values outside of range and "try again"
> 
> This does what I described in my last comment.  It throws away values outside
> of the range and keeps trying until it can return a valid number:
> http://www.prism.gatech.edu/~gtg037s/files/normal-variable.eps
> 
> Same test case, same normal 10,2 distribution.  I would push, but this needs
> some feedback from our new maintainer Michele Weigle.

This looks like a good solution to the issue.

changeset: d0c70dbe918e

Hi all,

The fix for the bounds that was applied is incorrect, or I am seriously
confused.  It currently, quite frequently, generates values outside the bounds.
 I am not sure if the original code had this behavior or not.  The below
snippet will bomb pretty much every run.  I suspect that the readjustment of
m_static_next if found out-of-bounds is incorrect, but have not investigated
further.

Thx


  bool bomb = 0;
  double mean = 1.0, variance = 0.2, bound = 0.75;
  for (int x = 0; x < 50; x++) {
    double x = NormalVariable::GetSingleValue(mean,variance,bound);
    if (x < mean-bound || x > mean+bound) {
      bomb = true;
      std::cout << "** ";
    }
    std::cout << x << std::endl;
  }
  if (bomb)
    std::cout << std::endl << "BOMBED" << std::endl;

(In reply to comment #10)
> Hi all,
> 
> The fix for the bounds that was applied is incorrect, or I am seriously
> confused.  It currently, quite frequently, generates values outside the bounds.
>  I am not sure if the original code had this behavior or not.  The below
> snippet will bomb pretty much every run.  I suspect that the readjustment of
> m_static_next if found out-of-bounds is incorrect, but have not investigated
> further.
> 
> Thx
> 
> 
>   bool bomb = 0;
>   double mean = 1.0, variance = 0.2, bound = 0.75;
>   for (int x = 0; x < 50; x++) {
>     double x = NormalVariable::GetSingleValue(mean,variance,bound);
>     if (x < mean-bound || x > mean+bound) {
>       bomb = true;
>       std::cout << "** ";
>     }
>     std::cout << x << std::endl;
>   }
>   if (bomb)
>     std::cout << std::endl << "BOMBED" << std::endl;


The previous patch did modify GetValue but not GetSingleValue.
> 

Created an attachment (id=176) [details]
fixes the static generator

This doesn't "BOMB" on Joe's test case, and uses the same logic that the
non-static variants use to generate bounded distributions.  Joe's distribution
with 300000 trials:
http://www.prism.gatech.edu/~gtg037s/files/joe-test-case.eps

Joe please do one final review, and lets push this bad boy.

This looks good to me.

For the record though, several of the other distributions do follow the
previously discussed policy of simply taking the bound as the value if it is
surpassed, rather than resampling (as NormalVariable now does).  Exponential,
Pareto, and Weibull all do that.

But, I would at least include this patch.

Thx Raj

Applied patch from 2008-06-19 16:14