[
https://issues.apache.org/jira/browse/MATH-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13427272#comment-13427272
]
Gilles commented on MATH-815:
-----------------------------
Hi Jared.
I've made some changes:
* formatting and missing Javadoc,
* removing unnecessary instance variables,
** DEFAULT_ABSOLUTE_ACCURACY
** covarianceMatrixEigenDecomposition
** covarianceMatrixEigenvalues
** covarianceMatrixEigenvectors
* reordering of the exception arguments,
* consistency checks (all rows of the covariance matrix must have the same
length).
The "main" code is ready for inclusion.
However, the "test" part is very hard to read, primarily because of the
"non-locality": e.g. a test for a trivial accessor like "getMeans" retrieves
its data not from a local variable but from data defined at the top of the file
and provided through a call to an abstract method!
In the absence of test code _defined_ in the abstract class, having such a
hierarchy is worse than useless.
Indeed, the point of having an abstract method like "makeDistribution" is to
design tests that are _independent_ of the distribution instance, whereas in
your test cases, you refer to the actual values used to construct the instance
returned by "makeDistribution". This is plainly the opposite of abstractness.
Please make a single, simple, self-contained
"MultivariateNormalDistributionTest", i.e. not extending an
"MultivariateRealDistributionAbstractTest" that serves no purpose in this case.
Note: When testing trivial functionality (like accessors), it is better to use
"simple" numbers (like 12.345 or 1.111111 etc.) and keep the number of
dimensions low (e.g. if "getMeans" returns the correct array in 2 dimensions,
it will likely do so in 10 or 100).
Also be careful with the tolerances: "getMeans" should retrieve the _same_
values that were passed to the constructor. Therefore, why do you use a large
tolerance like 1e-6? In that test, it should rather be something like 1e-16.
> Multivariate Normal Distribution
> --------------------------------
>
> Key: MATH-815
> URL: https://issues.apache.org/jira/browse/MATH-815
> Project: Commons Math
> Issue Type: New Feature
> Reporter: Jared Becksfort
> Priority: Minor
> Attachments: MultivariateNormalDistribution.java,
> MultivariateNormalDistributionTest.java, mvn.tgz, mvn2.tgz, mvn3.zip,
> mvn4.zip, patch
>
> Original Estimate: 1m
> Remaining Estimate: 1m
>
> I will submit a class for Multivariate Normal Distributions. Not sure if it
> will allow sampling initially.
> > Hello,
> >
> > I have implemented some classes for multivariate Normal distributions,
> > multivariate normal mixture models, and an expectation maximization fitting
> > class for the mixture model. I would like to submit it to Apache Commons
> > Math. I still have some touching up to do so that they fit the style
> > guidelines and implement the correct interfaces. Before I do so, I thought
> > I would at least ask if the developers of the project are interested in me
> > submitting them.
> >
> > Thanks,
> > Jared Becksfort
> Dear Jared,
> Yes, that would be very nice to have such an addition! Remember to also
> include unit tests (refer to the current ones for examples). The best would
> be to split a submission up into multiple minor ones, each covering a natural
> submission (e.g. multivariate Normal distribution in one submission), and
> create an issue as described at
> http://commons.apache.org/math/issue-tracking.html .
> If you run into any problems, please do not hesitate to ask on this mailing
> list.
> Cheers, Mikkel.
--
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
