commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luc Maisonobe <Luc.Maison...@free.fr>
Subject Re: [math] 1.2-RC1 available for review
Date Thu, 14 Feb 2008 21:26:32 GMT
Phil Steitz a écrit :
> 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!

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message