You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Ole Ersoy <ol...@gmail.com> on 2014/10/09 18:03:40 UTC
[Math] AbstractIntegerDistribution.public double logProbability(int
x) was Re: [Math] Separating Sampling from Distributions
>>> I think that, for some distribution, the computation of log(p) is more
>>> accurate. This was a feature request from not long ago.
>>>
>>
>> Just pasting the method below:
>>
>> public double logProbability(int x) {
>> return FastMath.log(probability(x));
>> }
>>
>> So it retrieves the probability of the index and uses FastMath to compute
>> the log. We could do:
>>
>> FastMath(distribution.probability(x));
>>
>> It just seems a bit over engineered to have something this trivial in the
>> API, unless I'm missing something.
>>
>
> This is the default implementation. For some distributions there is a more
> efficient way to compute the logProbability directly, and for these
> distributions the method is overridden (e.g. GeometricDistribution,
> BinomialDistribution, ...).
> Otherwise one would have to call probability(x) first, which in many cases
> involves calling exp(...) and then compute the log of it, which is just a
> waste of cpu.
So I guess my question is "Why is there value in having this as part of the public API of AbstractIntegerDistribution?". Not really a big deal, but it just seems like it's an implementation detail.
Ole
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] AbstractIntegerDistribution.inverseCumulativeDistribution
Unexpected Result
Posted by Ole Ersoy <ol...@gmail.com>.
> The variance of the DiceDistribution is wrong.
>
> Correct is the following: 2.91 or (70/24).
>
> You can verify the correct results also with a
> UniformIntegerDistribution(1, 6).
>
> Obviously the wrong variance was not noticed as the tests in this class
> do not compute the inverseCumulativeProbability.
I rewrote the tests and now they pass. Just wanted to share the precision info. If I make it smaller the tests that use it fail.
double precision = 0.000000000000001;
Assert.assertEquals(1, diceDistribution.inverseCumulativeProbability(0));
Assert.assertEquals(1, diceDistribution.inverseCumulativeProbability((1d-Double.MIN_VALUE)/6d));
Assert.assertEquals(2, diceDistribution.inverseCumulativeProbability((1d+precision)/6d));
Assert.assertEquals(2, diceDistribution.inverseCumulativeProbability((2d-Double.MIN_VALUE)/6d));
Assert.assertEquals(3, diceDistribution.inverseCumulativeProbability((2d+precision)/6d));
Assert.assertEquals(3, diceDistribution.inverseCumulativeProbability((3d-Double.MIN_VALUE)/6d));
Assert.assertEquals(4, diceDistribution.inverseCumulativeProbability((3d+precision)/6d));
Assert.assertEquals(4, diceDistribution.inverseCumulativeProbability((4d-Double.MIN_VALUE)/6d));
Assert.assertEquals(5, diceDistribution.inverseCumulativeProbability((4d+precision)/6d));
Assert.assertEquals(5, diceDistribution.inverseCumulativeProbability((5d-precision)/6d));//Can't use Double.MIN
Assert.assertEquals(6, diceDistribution.inverseCumulativeProbability((5d+precision)/6d));
Assert.assertEquals(6, diceDistribution.inverseCumulativeProbability((6d-precision)/6d));//Can't use Double.MIN
Assert.assertEquals(6, diceDistribution.inverseCumulativeProbability((6d)/6d));
Cheers,
- Ole
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] AbstractIntegerDistribution.inverseCumulativeDistribution
Unexpected Result
Posted by Ole Ersoy <ol...@gmail.com>.
> The variance of the DiceDistribution is wrong.
>
> Correct is the following: 2.91 or (70/24).
>
> You can verify the correct results also with a
> UniformIntegerDistribution(1, 6).
>
> Obviously the wrong variance was not noticed as the tests in this class
> do not compute the inverseCumulativeProbability.
Thanks Thomas! Do you want me to send a pull request with updates to the test?
Cheers,
- Ole
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] AbstractIntegerDistribution.inverseCumulativeDistribution
Unexpected Result
Posted by Thomas Neidhart <th...@gmail.com>.
On 10/10/2014 10:24 PM, Ole Ersoy wrote:
> Hi,
>
> I ran the following assertions on the dice distribution in the
> AbstractIntegerDistributionTest:
>
> Assert.assertEquals(3,
> diceDistribution.inverseCumulativeProbability(0.7d/6d)); //Expecting 1
> Assert.assertEquals(3,
> diceDistribution.inverseCumulativeProbability(0.8d/6d)); //Expecting 1
> Assert.assertEquals(3,
> diceDistribution.inverseCumulativeProbability(0.9d/6d)); //Expecting 1
> Assert.assertEquals(3,
> diceDistribution.inverseCumulativeProbability(1d/6d)); //Expecting 2
> Assert.assertEquals(3,
> diceDistribution.inverseCumulativeProbability(1.5d/6d)); //Expecting 2
> Assert.assertEquals(3,
> diceDistribution.inverseCumulativeProbability(2d/6d)); //Expecting 2
>
> Assert.assertEquals(3,
> diceDistribution.inverseCumulativeProbability(2.9d/6d)); //Expecting 3
> Assert.assertEquals(3,
> diceDistribution.inverseCumulativeProbability(3.0d/6d)); //Expecting 4
> Assert.assertEquals(4,
> diceDistribution.inverseCumulativeProbability(3.1d/6d));
>
> Assert.assertEquals(4,
> diceDistribution.inverseCumulativeProbability(3.2d/6d));
> Assert.assertEquals(4,
> diceDistribution.inverseCumulativeProbability(3.5d/6d));
>
> Assert.assertEquals(4,
> diceDistribution.inverseCumulativeProbability(4.0d/6d)); //Expecting 5
> Assert.assertEquals(4,
> diceDistribution.inverseCumulativeProbability(5.0d/6d)); //Expecting 5
> Assert.assertEquals(4,
> diceDistribution.inverseCumulativeProbability(5.1d/6d)); //Expecting 5
> Assert.assertEquals(5,
> diceDistribution.inverseCumulativeProbability(5.5d/6d));
> Assert.assertEquals(6,
> diceDistribution.inverseCumulativeProbability(6.0d/6d));
>
> All of them pass. Thoughts?
The variance of the DiceDistribution is wrong.
Correct is the following: 2.91 or (70/24).
You can verify the correct results also with a
UniformIntegerDistribution(1, 6).
Obviously the wrong variance was not noticed as the tests in this class
do not compute the inverseCumulativeProbability.
Thomas
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[Math] AbstractIntegerDistribution.inverseCumulativeDistribution
Unexpected Result
Posted by Ole Ersoy <ol...@gmail.com>.
Hi,
I ran the following assertions on the dice distribution in the AbstractIntegerDistributionTest:
Assert.assertEquals(3, diceDistribution.inverseCumulativeProbability(0.7d/6d)); //Expecting 1
Assert.assertEquals(3, diceDistribution.inverseCumulativeProbability(0.8d/6d)); //Expecting 1
Assert.assertEquals(3, diceDistribution.inverseCumulativeProbability(0.9d/6d)); //Expecting 1
Assert.assertEquals(3, diceDistribution.inverseCumulativeProbability(1d/6d)); //Expecting 2
Assert.assertEquals(3, diceDistribution.inverseCumulativeProbability(1.5d/6d)); //Expecting 2
Assert.assertEquals(3, diceDistribution.inverseCumulativeProbability(2d/6d)); //Expecting 2
Assert.assertEquals(3, diceDistribution.inverseCumulativeProbability(2.9d/6d)); //Expecting 3
Assert.assertEquals(3, diceDistribution.inverseCumulativeProbability(3.0d/6d)); //Expecting 4
Assert.assertEquals(4, diceDistribution.inverseCumulativeProbability(3.1d/6d));
Assert.assertEquals(4, diceDistribution.inverseCumulativeProbability(3.2d/6d));
Assert.assertEquals(4, diceDistribution.inverseCumulativeProbability(3.5d/6d));
Assert.assertEquals(4, diceDistribution.inverseCumulativeProbability(4.0d/6d)); //Expecting 5
Assert.assertEquals(4, diceDistribution.inverseCumulativeProbability(5.0d/6d)); //Expecting 5
Assert.assertEquals(4, diceDistribution.inverseCumulativeProbability(5.1d/6d)); //Expecting 5
Assert.assertEquals(5, diceDistribution.inverseCumulativeProbability(5.5d/6d));
Assert.assertEquals(6, diceDistribution.inverseCumulativeProbability(6.0d/6d));
All of them pass. Thoughts?
Cheers,
- Ole
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] AbstractIntegerDistribution.public double logProbability(int
x) was Re: [Math] Separating Sampling from Distributions
Posted by Ole Ersoy <ol...@gmail.com>.
> We could not yet add it to the interface due to binary / source
> compatibility rules, but it will be done for 4.0, see the ticket:
> https://issues.apache.org/jira/browse/MATH-1039
Just one more question/suggestion (Hopefully better than the earlier 50K one :) ) - would it make sense to have the distributions that have the log functionality implement an interface instead of inheriting the interface / default implementation?
Also, not sure if this is frowned on, but it would be great if there was a link to the issue (Or at least a note like "see MATH-1039") that contained the patch in such an interface, such that it is more self documenting.
Cheers,
- Ole
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] AbstractIntegerDistribution.public double logProbability(int
x) was Re: [Math] Separating Sampling from Distributions
Posted by Ole Ersoy <ol...@gmail.com>.
>> We could not yet add it to the interface due to binary / source
>> compatibility rules, but it will be done for 4.0, see the ticket:
>> https://issues.apache.org/jira/browse/MATH-1039
>
> OK - Thanks for the explanation. Based on the description in the ticket it seems like it would be better to have it as part of a utility, but that's just my 50K foot view.
Ooops - foot in mouth moment - looked at the patch - looks very very good - sorry for the noise.
Cheers,
Ole
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] AbstractIntegerDistribution.public double logProbability(int
x) was Re: [Math] Separating Sampling from Distributions
Posted by Ole Ersoy <ol...@gmail.com>.
On 10/09/2014 11:07 AM, Thomas Neidhart wrote:
> On Thu, Oct 9, 2014 at 6:03 PM, Ole Ersoy <ol...@gmail.com> wrote:
>
>> I think that, for some distribution, the computation of log(p) is more
>>>>> accurate. This was a feature request from not long ago.
>>>>>
>>>>>
>>>> Just pasting the method below:
>>>>
>>>> public double logProbability(int x) {
>>>> return FastMath.log(probability(x));
>>>> }
>>>>
>>>> So it retrieves the probability of the index and uses FastMath to compute
>>>> the log. We could do:
>>>>
>>>> FastMath(distribution.probability(x));
>>>>
>>>> It just seems a bit over engineered to have something this trivial in the
>>>> API, unless I'm missing something.
>>>>
>>>>
>>> This is the default implementation. For some distributions there is a more
>>> efficient way to compute the logProbability directly, and for these
>>> distributions the method is overridden (e.g. GeometricDistribution,
>>> BinomialDistribution, ...).
>>> Otherwise one would have to call probability(x) first, which in many cases
>>> involves calling exp(...) and then compute the log of it, which is just a
>>> waste of cpu.
>>>
>>
>> So I guess my question is "Why is there value in having this as part of
>> the public API of AbstractIntegerDistribution?". Not really a big deal,
>> but it just seems like it's an implementation detail.
>>
>
> We could not yet add it to the interface due to binary / source
> compatibility rules, but it will be done for 4.0, see the ticket:
> https://issues.apache.org/jira/browse/MATH-1039
OK - Thanks for the explanation. Based on the description in the ticket it seems like it would be better to have it as part of a utility, but that's just my 50K foot view.
Cheers,
- Ole
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] AbstractIntegerDistribution.public double
logProbability(int x) was Re: [Math] Separating Sampling from Distributions
Posted by Thomas Neidhart <th...@gmail.com>.
On Thu, Oct 9, 2014 at 6:03 PM, Ole Ersoy <ol...@gmail.com> wrote:
> I think that, for some distribution, the computation of log(p) is more
>>>> accurate. This was a feature request from not long ago.
>>>>
>>>>
>>> Just pasting the method below:
>>>
>>> public double logProbability(int x) {
>>> return FastMath.log(probability(x));
>>> }
>>>
>>> So it retrieves the probability of the index and uses FastMath to compute
>>> the log. We could do:
>>>
>>> FastMath(distribution.probability(x));
>>>
>>> It just seems a bit over engineered to have something this trivial in the
>>> API, unless I'm missing something.
>>>
>>>
>> This is the default implementation. For some distributions there is a more
>> efficient way to compute the logProbability directly, and for these
>> distributions the method is overridden (e.g. GeometricDistribution,
>> BinomialDistribution, ...).
>> Otherwise one would have to call probability(x) first, which in many cases
>> involves calling exp(...) and then compute the log of it, which is just a
>> waste of cpu.
>>
>
> So I guess my question is "Why is there value in having this as part of
> the public API of AbstractIntegerDistribution?". Not really a big deal,
> but it just seems like it's an implementation detail.
>
We could not yet add it to the interface due to binary / source
compatibility rules, but it will be done for 4.0, see the ticket:
https://issues.apache.org/jira/browse/MATH-1039
Thomas