commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <stei...@yahoo.com>
Subject Re: cvs commit: jakarta-commons-sandbox/math/src/java/org/apache/commons/math/stat UnivariateImpl.java
Date Tue, 17 Jun 2003 15:50:16 GMT

--- Tim O'Brien <tobrien@discursive.com> wrote:
> On Tue, 17 Jun 2003, Mark R. Diggory wrote:
> 
> > -1,
> 
> Mark, all action items are subject to lazy consensus.  Please roll back
> the changes until a consensus has formed.  These are the guidelines of
> Jakarta - we operate by these rules.  Action items subjet to lazy 
> consensus can be -1'd, if they are it is responsibility of the committer 
> to roll back and engage the community in discussion.
> 
> > You should review my code changes. My decisions are clearly more 
> > benificial for the UnivariateImpl, this is because, when UnivariateImpl 
> > is instantiated with a window, it is no longer a storageless solution, 
> > one shouldn't totally rewrite the storage based approach when this case 
> > arises, one should deligate to a standard implementation for storage 
> > based solutions thats located in one abstract implementation (as you 
> > have done for the other storage based solutions). 

We all agree that the optimized double[]|-> double computations should be
reused.  The problem with the above is that you can't "dynamically" reorganize
class hierarchies.  UnivariateImpl either "is" an AbstractStoreUnivariate or it
"is not".  IMHO, it is not.  If we want to define an abstract base class for
everything, it needs to contain only the things that *all* Univariates *always*
implement.  Throwing runtime exceptions when an instance is not a "full
instance" is not acceptable.   

As I see it, we have two choices.  The simplest is to have both
AbstractStoreUnivariate and UnivariateImpl use computational methods
encapsulated in StatUtils.  This would have the advantageous side effect of
exposing direct fast, efficient, optimized mean, variance computations for use
elsewhere in commons-math and by users.  I will likely submit these things in
any case, if no one else does.  It might even make sense to add "updating"
methods that take lagged statistics as arguments.  These we do not need
immediately.

The second choice (inferior, IMHO because of limitations on reuse and
unecessary additional layers of abstraction) would be to add an
AbstractUnivariate class that includes *only* the methods that will *always* be
available -- i.e. what is in Univariate.  AbstractStoreUnivariate would then
have to extend this, as would UnivariateImpl.  UnivariateImpl would have to
make a dynamic decision of whether or not to override the array-based
computation. I do not like this solution, but I could support it if strong
arguments are presented as to why the StatUtils approach is not preferable.  So
far, I see no reason why we should not go with the simpler approach.

I have also noticed that somehow the getWindowSize() method has been added to
the Univariate interface.  Why did we do this?  I do not see this as part of
Univariate.  The window size is a property of UnivariateImpl, not a generic
property.

Phil

This makes sense 
> > because all AbstractStoreUnivariate is implmeneting is a "lightweight" 
> > set of methods that opperate independently of the internal storage 
> > solution. This is elegant and should be used as well in UnivariateImpl.
> 
> Your motives are good - you want to encourage reuse of existing logic, but
> the implementation leaves something to be desired.
> 
> There are measures which are impossible with a storageless Univariate.  
> Take Quartiles as only one example. On the other hand, Univariate as an
> interface collects all measures for storageless implementations (a subset
> of StoreUnivariate).  StoreUnivariate extends Univariate and adds measures
> which can only be returned for storage-based implementations.  The change
> you made to UnivariateImpl extends AbstractStore---.
> 
> The hierachy for UnivariateImpl is now Univariate <-- StoreUnivariate <-- 
> AbstractStoreUnivariate <-- UImpl AND 
> Univariate <-- UnivariateImpl.   UnivariateImpl now throws a 
> RuntimeException for certain measures depending on whether or not we have 
> a window size defined.  This is confusing.
> 
> I could care less about my toes, I'm only interested in the code, and 
> UnivariateImpl as it stands right now is confusing. 
> 
 > 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
> 


__________________________________
Do you Yahoo!?
SBC Yahoo! DSL - Now only $29.95 per month!
http://sbc.yahoo.com

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


Mime
View raw message