You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Ryan Gaffney (JIRA)" <ji...@apache.org> on 2016/05/31 01:41:12 UTC
[jira] [Comment Edited] (MATH-1370) Increase efficiency of
EnumeratedDistribution#probability
[ https://issues.apache.org/jira/browse/MATH-1370?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15307011#comment-15307011 ]
Ryan Gaffney edited comment on MATH-1370 at 5/31/16 1:41 AM:
-------------------------------------------------------------
[~erans]
{quote}Is the additional unit test (in a new file) really necessary?
Does it test more than EnumeratedIntegerDistributionTest?{quote}
It is also testing the {{getPmf()}} function which is specific to EnumeratedDistribution. (I did not want to break the behavior for nulls)
From the JavaDoc: {{Note that if duplicate and / or null values were provided to the constructor when creating this EnumeratedDistribution, the returned list will contain these values.}}
{quote}Do you agree that the method probability defined as follows: [...]{quote}
-Along the same lines as my previous response, the integer distribution classes allow for null values. With the change you would always return {{0}} for {{null}} even though that might be a valid random variable. It would therefore break the added test on L47.-
-_Note that I was just keeping parity with the original code; I am not arguing over whether or not we should allow null random variables_ :)-
Disregard, see [comment below|https://issues.apache.org/jira/browse/MATH-1370?focusedCommentId=15307069&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15307069]
was (Author: rg_):
[~erans]
{quote}Is the additional unit test (in a new file) really necessary?
Does it test more than EnumeratedIntegerDistributionTest?{quote}
It is also testing the {{getPmf()}} function which is specific to EnumeratedDistribution. (I did not want to break the behavior for nulls)
From the JavaDoc: {{Note that if duplicate and / or null values were provided to the constructor when creating this EnumeratedDistribution, the returned list will contain these values.}}
{quote}Do you agree that the method probability defined as follows: [...]{quote}
Along the same lines as my previous response, the integer distribution classes allow for null values. With the change you would always return {{0}} for {{null}} even though that might be a valid random variable. It would therefore break the added test on L47.
_Note that I was just keeping parity with the original code; I am not arguing over whether or not we should allow null random variables_ :)
> Increase efficiency of EnumeratedDistribution#probability
> ---------------------------------------------------------
>
> Key: MATH-1370
> URL: https://issues.apache.org/jira/browse/MATH-1370
> Project: Commons Math
> Issue Type: Improvement
> Reporter: Ryan Gaffney
> Priority: Minor
> Labels: patch, performance
> Attachments: enum-distribution-perf-patch, enum-distribution-perf-patch.2
>
>
> There are lots of other low hanging fruit in the distribution package but unfortunately this is the only one I got to that day.
> In the EnumeratedDistribution case, the probability() method is currently O(N) where N denotes number of random variables, even though the random variables are fixed / known. This change makes probability() O(1). The unlikely worst case scenario now consumes slightly more than 2x the memory, but I do not think this would be an issue in the vast majority of cases.
> Original PR (incorrectly against master) [here|https://github.com/apache/commons-math/pull/34].
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)