[prev in list] [next in list] [prev in thread] [next in thread] 

List:       jakarta-commons-dev
Subject:    Re: [STATISTICS] Remove isSupportConnected from distribution interface
From:       Gilles Sadowski <gilleseran () gmail ! com>
Date:       2021-11-30 23:14:57
Message-ID: CACioL_Ei=_aMycyNFy38wovBmeju=dTm3k+Yec9zuYZ=r-mN9A () mail ! gmail ! com
[Download RAW message or body]

Le mar. 30 nov. 2021 à 19:41, Alex Herbert <alex.d.herbert@gmail.com> a écrit :
> 
> On Tue, 30 Nov 2021 at 16:50, Gilles Sadowski <gilleseran@gmail.com> wrote:
> > 
> > Hello.
> > 
> > Sorry for being late, but the change[1] in commit
> > d7c53ab3e05b09f2522f7b7ee78e96ad97defe95
> > impacts Commons Math: Method "isSupportConnected" is called in class
> > "AbtractRealDistribution".[2]
> > That class was ported to [STATISTICS] as "AbstractContinuousDistribution".
> > So now there are code duplications (beacuse ""AbtractRealDistribution" is
> > not public).  Perhaps the "searchPlateau" functionality could be provided in
> > a public API (?).
> 
> I missed the fact that AbstractContinuousDistribution is not public.
> This makes the case for removing the isSupportConnected functionality
> stronger. I left it in so that it could be used by any extending
> classes but there are none in Commons Statistics. So the class
> supports functionality that is not used and cannot be inherited.
> 
> Note that isSupportConnected is not used by anything in Commons Math
> either.

OK, I missed that.  So it's just a question of updating the code there.

> It is also a rather poor implementation as it performs
> bisection between the current value and the lower bound. This may be
> very far from the current value and so the search for the connected
> support will be slow. I did not try and improve it as the function is
> not used.
> 
> So here are some options:
> 
> 1. The functionality in Commons Math can be deleted. A quick look and
> it appears the AbtractRealDistribution can also be made package
> private.

+1

> 2. AbstractContinuousDistribution in Statistics can be made public.
> 3. The functionality from AbstractContinuousDistribution can be
> transferred to Commons Math.
> 
> I am leaning towards option 1. The method can live on in statistics in
> the event it is useful in the future.
> 
> > 
> > Side-note: Why wasn't the issue caught by Jenkins?
> > Commons Math is not listed as being dependent on it.[3]
> 
> So should we update Jenkins to have math build on changes to
> Statistics, Numbers and RNG?

Sure.  I thought that it was inferred from the POM file...

Thanks,
Gilles

> 
> Alex
> 
> > 
> > Regards,
> > Gilles
> > 
> > [1] https://issues.apache.org/jira/projects/STATISTICS/issues/STATISTICS-48
> > [2] https://gitbox.apache.org/repos/asf?p=commons-math.git;a=blob;f=commons-math-l \
> > egacy/src/main/java/org/apache/commons/math4/legacy/distribution/AbstractRealDistribution.java;h=3d79914bc6c19be3de255ce53763c1cae479ac33;hb=HEAD#l172
> >  [3] https://ci-builds.apache.org/job/Commons/job/commons-statistics/
> > 

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


[prev in list] [next in list] [prev in thread] [next in thread]