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: svn commit: r1086057 - in /commons/proper/math/trunk/src: main/java/org/apache/commons/math/stat/correlation/PearsonsCorrelation.java test/java/org/apache/commons/math/stat/correlation/PearsonsCorrelationTest.java
Date Tue, 29 Mar 2011 16:28:14 GMT
Le 29/03/2011 17:16, sebb a écrit :
> On 29 March 2011 16:01, Gilles Sadowski <gilles@harfang.homelinux.org> wrote:
>>>>>> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/correlation/PearsonsCorrelation.java
>>>>>> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/correlation/PearsonsCorrelation.java?rev=1086057&r1=1086056&r2=1086057&view=diff
>>>>>> ==============================================================================
>>>>>> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/correlation/PearsonsCorrelation.java
(original)
>>>>>> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/correlation/PearsonsCorrelation.java
Sun Mar 27 22:06:18 2011
>>>>>> @@ -225,7 +225,8 @@ public class PearsonsCorrelation {
>>>>>>       * @throws  IllegalArgumentException if the arrays lengths do
not match or
>>>>>>       * there is insufficient data
>>>>>>       */
>>>>>> -    public double correlation(final double[] xArray, final double[]
yArray) throws IllegalArgumentException {
>>>>>> +    public static double correlation(final double[] xArray, final
double[] yArray)
>>>>>> +        throws IllegalArgumentException {
>>>>>>          SimpleRegression regression = new SimpleRegression();
>>>>>>          if (xArray.length != yArray.length) {
>>>>>>              throw new DimensionMismatchException(xArray.length,
yArray.length);
>>>> As is, it is not clear (from a design point-of-view) that it would make
>>>> sense to override the method (hence the legitimate request to make it
>>>> static). If polymorphism is really the intention, I think that it would
>>>> be worth adding an interface ("Correlation" maybe?).
>>>
>>> We have been talking about moving away from interfaces as the
>>> preferred way to support people plugging in alternative
>>> implementations because they have in several places gotten "behind"
>>> due to the fact that adding anything to them breaks compatibility.
>>> We should probably continue that discussion in a different thread.
>>
>> OK, comments kept for a separate thread...
>>
>>>
>>> The method above is the core of the implementation
>>
>> One is left wondering why the "core" does not need anything else from the
>> class (no fields, no methods); it is self-sufficient (hence the legitimate
>> request for making it static).

Even without field or method, it may be interesting to have instances to
distinguish classes. The added benefit is that when we add methods, we
can do so in upward compatible ways.

>>
>>> and making it
>>> static means that you can't extend this class using a different
>>> implementation.
>>
>> Is that a problem (other than the practical one you invoke just below).
>>
>>> It also breaks all existing code using the class.
>>> There are lots of other examples in [math] - throughout Commons
>>> actually - where methods *could* be static, but are not because we
>>> want to preserve the ability for ourselves or users to extend the
>>> classes and override the methods.
>>
>> The issue which I'm raising is whether it is functionally clearer (as in
>> "How the class is _supposed_ to be used") to have (or not) a static method.
>> If the common case is inheritance (of which there is no example in CM) then
>> there must be an interface to single out the "core" functionality.
> 
> Insisting on an interface means updates are binary incompatible unless
> the interface is for internal use only.
> 
> AFAICT, the only way to ensure bin.com. is to provide a base class
> (probably abstract) which users can subclass.
> 
> Or, one has to add an new interface for each new API, and one would
> probably still need a base class.
> 
> It's very difficult getting an interface right the first time.

Yes, the 2.X fiasco clearly shows that. We failed miserably on some
interfaces.

> 
>> If instead, we focus on the specificity of the implementation, a user can
>> always "extend" the functionality by composition (i.e. his new class will
>> wrap a call to the static "PearsonsCorrelation.correlation" method).

We often want the inheritance to be visible and public, not hidden and
wrapped.

Luc

>>
>>> All of the evaluate() methods,
>>> for example, in the individual descriptive statistics, for example,
>>> could be static.  Convenience methods are provided in StatUtils to
>>> support static invocation.  I think a CorrelationUtils class for
>>> this, Covariance and Spearman's - which all have core implementation
>>> methods that "could" be static - is a better approach if we feel
>>> that we need to provide support for the procedural API style here.
>>
>> That's not only an issue of providing procedural style API.
>>
>>
>> Gilles
>>
>> ---------------------------------------------------------------------
>> 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
> 


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


Mime
View raw message