commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Phil Steitz" <phil.ste...@gmail.com>
Subject Re: [math] 1.2-RC1 available for review
Date Thu, 14 Feb 2008 00:40:59 GMT
On Feb 12, 2008 7:25 PM, Michael Heuer <heuermh@acm.org> 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!
>
> 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.


> complex
>
> Complex and ComplexFormat could be final
>
Not a good idea at this time, IMO.

> estimation
>
> AbstractEstimator has several protected fields and public methods that
> are not final

Here again,  -1 on making these final.

> SimpleEstimationProblem ArrayList unbound --> List unbound
> SimpleEstimationProblem fields private ArrayList --> private final List

Sounds reasonable.  Any problems with these changes, Luc?

>
> fraction
>
> Fraction and FractionFormat could be final

-1 for 1.2 at least.
>
> geometry
>
> Rotation minor javadoc fix "if axis norm is null" vs if (norm == 0) { ... }

Patch welcome.

> 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?

> 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?

> optimization
>
> method name change, DirectSearchOptimizer.minimizes(... --> minimize(...
>
Personally +1 on this change, but again need to think about Mantissa users.

> 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.

> 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?
>
> 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: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message