Phil Steitz a écrit :
On Feb 12, 2008 7:25 PM, Michael Heuer <[EMAIL PROTECTED]> wrote:
On Sun, 10 Feb 2008, Phil Steitz wrote:
The zips / tars are here:
http://people.apache.org/~psteitz/math-1.2-RC1/
The site included in the binary distro is here:
http://people.apache.org/~psteitz/math-1.2-RC1/docs/
Release notes:
http://people.apache.org/~psteitz/math-1.2-RC1/RELEASE-NOTES.txt
Ant, Maven 1 and Maven 2 builds should all work from the unpacked
source distribution.
Comments / suggestions for improvement welcome!
Phil
A few comments for consideration:
Thanks, Michael!
Thanks Michael! We're happy to see such deep analysis.
Null parameter checks are not consistent.
Some classes throw NPE (PolynomialFunction, Complex, ComplexUtils,
RealMatrixImpl, MatrixUtils, etc.)
Some classes throw IAE (UnivariateRealSolverUtils,
StorelessUnivariateStatistic, StatUtils, etc.)
Many classes do neither (AbstractEstimator, GaussNewtonEstimator,
LevenbergMarquardEstimator, Rotation, RotationOrder, BigMatrix,
BigMatrixImpl, QRDecompositionImpl, RealMatrix, RealMatrixImpl,
AbstractStepInterpolator, DirectSearchOptimizer,
CorrelatedRandomVectorGenerator, EmpiricalDistribution,
EmpiricalDistributionImpl, RandomData, RandomDataImpl,
UncorrelatedRandomVectorGenerator, VectorialCovariance, VectorialMean,
etc.)
e.g. EmpiricalDistributionImpl.StreamDataAdapter ctr is especially
problematic since NPE wouldn't be thrown until later, when either
computeBinStats or computeStats was called
Patches welcome on the last one. The others need to be looked at and
discussed individually if we want to change behavior and API
documentation. Javadoc patches welcome.
As far as I know, the only place where NPE is explicitely thrown is in
Complex.pow. All other references to NPE I found in the
src/java/**/*.java file are only in the javadoc comments. They do not
correspond to checks performed at [math] level. This seems quite
reasonable to me.
The case for IllegalArgumentException being thrown for null parameters
is different and must be addressed on a case by case basis. I agree this
is inconsistent, so we should either still perform the check and throw
the exception ourselves (just like Complex.pow does) or let the JVM
throw the exception automatically and not specify it in the javadoc.
However, this would be an API change and cannot be done in 1.2. We have
to wait for 2.0 for such changes.
Could you open a JIRA ticket with an objective fix set to 2.0 for this
issue ?
complex
Complex and ComplexFormat could be final
Not a good idea at this time, IMO.
This would break compatibility. Once again, wait for 2.0. This is the
same sort of problem we encountered with the TOO_SMALL fields in the
matrix classes and which we cannot fix now for compatibility sake.
estimation
AbstractEstimator has several protected fields and public methods that
are not final
Here again, -1 on making these final.
There are no compatibility issues here since this class is new in
[math]. The maxCostEval and costEvaluations fields should be private and
the corresponding accessors should be final (they are public). Im am +1
on these two.
I'm not really sure about other fields and methods. Having private
fields and protected final accessors seems unnecessary verbose to me.
Those fields are really useful to the implementation classes and they
can change depending on the way the user calls the library (I sometime
use solvers in a loop and change the settings, sometimes even changing
the number of parameters or measurements between iterations).
SimpleEstimationProblem ArrayList unbound --> List unbound
SimpleEstimationProblem fields private ArrayList --> private final List
Sounds reasonable. Any problems with these changes, Luc?
Why not ? Neither ArrayList nor List appear anywhere on the public
interface, this is really an implementation detail. If you prefer using
List, go ahead.
Declaring the fields to be final is a good idea.
fraction
Fraction and FractionFormat could be final
-1 for 1.2 at least.
I don't understand. [math] is a quite low level library, so we really
cannot know how it will be used and if users won't need to extends the
classes we propose.
The FractionFormat could be guaranteed to be immutable and the two
format fields could be made final in 2.0, though.
geometry
Rotation minor javadoc fix "if axis norm is null" vs if (norm == 0) { ... }
Patch welcome.
Good catch ! This appears at several places and should also be changed
in the exception error messages.
the API of Vector3D should be similar to RealMatrix/BigMatrix and
follow the same implementation pattern
Good point, but I am OK with the inconsistency and I suspect Luc and
migrating Mantissa users would rather keep this as is. Luc?
This API is quite consistent as is, the getNorm(), add()and subtract()
methods are similar to the methods with the same names in the linear
package. The multiply(double) method is similar to the
scalarMultiply(double) method and has a name inconsistency. The other
methods are specific to this class.
If you consider changing multiply() to scalarMultiply() would bring a
more consistent package, then go for it, it is the right to do this
before we release the class in [math] for the first time. This is a
small change with regard to the package changes mantissa users will have
to do. If you want more important changes like removing some methods
like add and subtract with factor and vector, or generalizing the class
to any dimension or replacing the getX()/getY()/getZ() by
getEntry(index), then I would strongly argue against it.
linear
BigMatrixImpl minor javadoc fix (data vs. d parameter name)
RealMatrixImpl minor javadoc fix (data vs. d parameter name)
Patch welcome
ode
AbstractStepInterpolator has several methods that are protected and
modify private state or that are public and not final
Comments on this, Luc?
The only private parts of the state are the finalized and forward
boolean fields. They are modified by the protected constructors and
reinitialize (which acts as a disguised constructor called as part of
the prototype design pattern used in Runge-Kutta and embedded
Runge-Kutta implementations). The finalized field is also modified by
the public finalizeStep method which is final. All of this is
intentional. The weirdest thing is the prototype design pattern and the
reinitialize method, but I didn't find a clean way to do what I wanted.
What do you suggest here and for which fields ?
optimization
method name change, DirectSearchOptimizer.minimizes(... --> minimize(...
Personally +1 on this change, but again need to think about Mantissa users.
You are right, and it is also a small change for Mantissa users, go ahead.
random
refactor the decomposition in CorrelatedRandomVectorGenerator to a
separate class in linear package
+1 but this is a little work. Could wait for 2.0, though this amounts
to release-with-intention-to-deprecate.
I agree this slightly modified Cholesky decomposition may be useful
elsewhere, taking care of clashes with regular Cholesky decomposition.
Here, we add permutations and a stop condition on small vectors to
handle rank-deficient matrices (i.e. matrices that are only
semi-definite positive, not definite positive).
This could wait for 2.0 as a new feature with more bells and whistles.
There is no need to deprecate anything as it is a private method
modifying a private field.
ValueServer replace static ints with typesafe enum, minor javadoc fixes
(periods at end of sentences, etc.);
Patches welcome
>candidate for its own package, with
interface and multiple implementations
Can do this in 2.0.
transform
better documentation as to difference between transform and transform2
and between inversetransform and inversetransform2
method name change, inversetransform --> inverseTransform
method name change, inversetransform2 --> inverseTransform2
+1 on these. Any objections, Luc?
No problem.
Luc
util
DefaultTransformer, NumberTransformer, TransformerMap not related to the
transform package desipe their names, better to delegate to
commons-convert? (or possibly use a similar pattern, e.g.
DoubleToStringConversion)
These should be deprecated.
For "later":
Generify matrix API
Package interfaces and implementation classes separately
+1 for 2.0
I realize that some of these cannot happen in a 1.2 release because of
API compatibility. I can provide patches for the others if desired.
Thanks again for the review and feedback and thanks in advance for patches.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
