[ 
https://issues.apache.org/jira/browse/MATH-703?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13157001#comment-13157001
 ] 

Sébastien Brisard commented on MATH-703:
----------------------------------------

Hi Christian,
I've reviewed your patch, and have some concerns
* in class {{AbstractRealDistribution}}, the mean and variance of the actual 
distribution is cached, which is good, but I think that nothing guarantees 
immutability of classes inheriting {{AbstractRealDistribution}}. For example, I 
might implement my own normal distribution, with {{setMean(double)}} and 
{{setVariance(double)}} methods, and I would then need to make sure the proper 
private fields in the {{AbstractRealDistribution}} are updated accordingly. 
While caching should be allowed for in this hierarchy of classes, I think no 
default implementation of {{getNumericalMean()}} and {{getNumericalVariance()}} 
should be provided at this stage. This might be silly, please correct me if I'm 
wrong. I think people raised the same concerns about the dimensions of the 
domain and codomain of linear operators, when I first submitted an 
implementation where these dimensions where stored in the base abstract class.
* I'm not convinced about {{AbstractDefaultRealDistribution}} and 
{{AbstractDefaultIntegerDistribution}}: not having them would only mean that we 
would need to implement one or two additional methods, with very simple (in 
fact, constant, as you noticed) values. So we need to compare this slight 
inconvenience on the one hand with the encumbrance of the class hierarchy on 
the other hand... And in any case, people would *always* have to refer to the 
Javadoc of these default implementations to make sure that what they want to 
implement is indeed a {{AbstractDefaultXxxDistribution}}

These are only my views, though, and I'm going to post an invitation on the 
mailing-list in order to invite others to review your work.
                
> Splitting up the distribution hierarchy
> ---------------------------------------
>
>                 Key: MATH-703
>                 URL: https://issues.apache.org/jira/browse/MATH-703
>             Project: Commons Math
>          Issue Type: Improvement
>            Reporter: Christian Winter
>            Priority: Minor
>         Attachments: MATH-703_patch.zip
>
>
> As discussed on the mailing list 
> (http://apache-commons.680414.n4.nabble.com/math-Distributions-over-sample-spaces-other-than-R-tp3931349p3931349.html),
>  the distribution interfaces should be restructured.
> The most important point is to create one root interface for each domain. 
> There should *not* be a common super-interace because different domains 
> require different functionality. Additionally, a super-inferface would 
> require to parametrize the domain which makes things more complicated (e.g., 
> "double" would have to be replaced by "Double"). Currently, Commons Math 
> supports distributions with real domain and distributions with integer 
> domain. Thus there will be the interfaces RealDistribution and 
> IntegerDistribution.
> Another point is to drop the special cases of distributions with real domain 
> in order to simplify the structure. There won't be an interface for 
> absolutely continuous distributions, and there won't be an interface for 
> discrete distributions on the real domain. All the functionality required by 
> the special cases can be defined in RealDistribution.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


Reply via email to