Hi.
On Tue, 30 Jan 2018 10:02:03 +0100, Eric Barnhill wrote:
Overall I think the old math-statistics functioned well and I would
not be
inclined to mess with the old object hierarchy without reason.
There are good reasons:
https://issues.apache.org/jira/browse/MATH-1228
https://issues.apache.org/jira/browse/MATH-1281
Porting without fixing such things is nonsense.
But there
are some strange design choices in this code.
Strange designs should not stay without reason. ;-)
Mean() is used here as an
example.
1) In Mean() the two constructors create a FirstMoment() object:
--
/** Constructs a Mean. */
public Mean() {
incMoment = true;
moment = new FirstMoment();
}
/**
* Constructs a Mean with an External Moment.
*
* @param m1 the moment
*/
public Mean(final FirstMoment m1) {
this.moment = m1;
incMoment = false;
}
--
"FirstMoment" is mentioned in the above JIRA issue.
And then this moment object, as far as I can tell, is never used in
the
mean calculation at all:
---
@Override
public double evaluate(final double[] values, final int begin,
final
int length)
throws MathIllegalArgumentException {
if (MathArrays.verifyValues(values, begin, length)) {
Sum sum = new Sum();
double sampleSize = length;
// Compute initial estimate using definitional formula
double xbar = sum.evaluate(values, begin, length) /
sampleSize;
// Compute correction factor in second pass
double correction = 0;
for (int i = begin; i < begin + length; i++) {
correction += values[i] - xbar;
}
return xbar + (correction/sampleSize);
}
return Double.NaN;
}
---
Indeed, problem 1 is, as you note below, that an instance method
is used to compute the mean of external data. It should have been
"static".
Another possibility is to pass the data to the instance, overloading
"increment":
void increment(double[] values, int startIndex, int len);
FirstMoment just seems to be in the constructor for its own sake.
Not so; "moment" is used in "getResult()".
IIRC, the constructor you refer to enables aggregation.
But this seems an ad-hoc addition to the API (and the cause of a bug,
as the OP reported on JIRA).
I think it would make more sense to at the very least make Mean and
similar classes very lean, and see if there are any use cases for
which
this is not sufficient.
So indeed, you suggest to either change the design or drop some
of the current functionality (aggregation).
2)
Mean extends AbstractStorelessUnivariateStatistics. I have no
problem with
the class being Storeless, which I think means immutable.
Not so: e.g. in "Mean", the "moment" field is
* "protected",
* not "final", and
* its class ("FirstMoment") contains non-final fields.
We can't be farther from immutability.
In the new API, "protected" should be avoided unless there is a
good (documented) reason. ;-)
But, in that case
the methods might as well be static. However the old grammar of
new Mean().evaluate()
is only marginally different from what the static grammar would be:
Mean.evaluate
The latter is the right choice, given the semantics of the method.
However, if object creation is assumed to be cheap, the better
OO alternative is ti pass the data to a constructor and retrieve
the result:
m = new Mean(data).get();
Then, given that the project is targetting Java 8, the new API
must take advantage of that. Once it is "right", adapter can
be thought about to ease the transition of codes that used CM.
Concerning "evaluate", shouldn't it be based on streams?
Regards,
Gilles
-Eric
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
