You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Sébastien Brisard <se...@m4x.org> on 2011/11/05 09:21:25 UTC
[math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Hi,
while working on MATH-699 (which was triggered by MATH-692), I found
some inconsistencies in the use of
UnivariateRealSolver.getAbsoluteAccuracy() and
UnivariateRealSolver.getFunctionValueAccuracy() (see the JIRA ticket
for more details).
In short, the current implementation of AbstractContinuousDistribution
allows customization of the default solver (for inverting the CDF) via
AbstractContinuousDistribution.getSolverAbsoluteAccuracy(). To be
completely consistent, we would also need to provide a new method
AbstractContinuousDistribution.getSolverFunctionValueAccuracy().
Instead, I suggest we open the possibility to provide a custom solver
to AbstractContinuousDistribution (with all three tolerances set by
the user), as well as a getSolver() method.
What do you think of this change?
Sébastien
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Sébastien Brisard <se...@m4x.org>.
Dear Gilles and Phil,
just a quick note to invite you to review the addition I've made to
MATH-699. It turns out I'm struggling with finite precision: it is
difficult to make a consistent use of thresholds. For the time being,
the way I see it is that you are both right, but we can set the design
issues aside right now, because the logical use of these thresholds
requires discussion first. For example, I'm no longer convinced I need
getSolverFunctionValueAccuracy()... So detailed implementation of this
method does not really matter.
As written in MATH-699, I *will* need your help for a careful review
of the code, because something seemingly as simple as detecting a
plateau gives me some troubles. Also, I think we will have to contend
with an implementation which is not completely fool-proof.
Best regards,
Sébastien
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] replace
AbstractContinuousDistribution.getSolverAbsoluteAccuracy() with
AbstractContinuousDistribution.getSolver()
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
On Sun, Nov 06, 2011 at 10:47:31AM -0700, Phil Steitz wrote:
> On 11/6/11 10:24 AM, Gilles Sadowski wrote:
> > On Sun, Nov 06, 2011 at 09:28:48AM -0700, Phil Steitz wrote:
> >> On 11/6/11 7:59 AM, Gilles Sadowski wrote:
> >>> On Sun, Nov 06, 2011 at 12:24:03AM -0700, Phil Steitz wrote:
> >>>> On 11/6/11 12:18 AM, Sébastien Brisard wrote:
> >>>>>> +1, but your observation above then leads to the question where are
> >>>>>> you going to get this value from? There may not be a solver to read
> >>>>>> it from. I guess the default impl in the base class could just
> >>>>>> return BaseAbstractUnivariateRealSolver.DEFAULT_FUNCTION_VALUE_ACCURACY.
> >>>>>>
> >>>>>> Phil
> >>>>>>
> >>>>> Ah, that's one option I didn't think of. I intended to follow the same
> >>>>> scheme as for getSolverAbsoluteAccuracy(), which returns
> >>>>> solverAbsoluteAccuracy (private field), possibly initialized with
> >>>>> DEFAULT_SOLVER_ABSOLUTE_ACCURACY (see AbstractContinuousDistribution).
> >>>>> Actually, using the BaseAbstractUnivariateRealSolver default value is
> >>>>> probably better. However, these default values are private... So maybe
> >>>>> we will have to select reasonable default values here.
> >>>> Ugh. I remember complaining about that (the fact that the defaults
> >>>> were made private) for precisely this kind of reason. Have to clone
> >>>> the value, I guess and keep it in synch with whatever Brent uses.
> >>>>
> >>>> Phil
> >>>>> Sébastien
> >>> [I did not follow all the details of this discussion; sorry if I'm slightly
> >>> off base.] But, if somewhere some _default_ accuracy is needed to pass to a
> >>> _default_ solver, I'd say: instantiate the solver using its _default_
> >>> constructor; thus, no need to chase up instance variables used further up
> >>> the hierarchy.
> >> Read the post. What is being reused is the default value itself.
> >> There may or may not be a solver instantiated.
> > If there is no solver, why would you want to access an instance variable of
> > the solvers's base class?
>
> What the code could use is access to the *default* function value
> accuracy, essentially to reuse the default value. This is a reason
> that it is good to expose default configuration properties as public
> constants. What the code is doing is checking to see if the domain
> upper / lower bounds return values under the function that are
> within the default function value accuracy (logically a property of
> the distribution at this point). If this test succeeds, one of the
> domain endpoints will be returned and no solver will be
> instantiated. It would be nice from a consistency perspective to
> use the default that BaseAbstractUnivariateRealSolver defines.
On the one hand you say
"[...] default function value accuracy (logically a property of
the distribution at this point) [...]"
On the other:
"It would be nice from a consistency perspective to
use the default that BaseAbstractUnivariateRealSolver defines."
I don't see why it is logical that the default value in one context
would be a good default in another. [I'm not even sure that it is a good
thing to provide defaults for the solvers.].
>From my point-of-view, "BaseAbstractUnivariateRealSolver" is an
implementation detail which shouldn't even be known from a user's
perspective.
The defaults there were defined only to allow the subclasses to be "sloppy"
in that they don't have to define 2 out of the 3 "DEFAULT...", on the
assumption (which may not always be correct) that for a solver instantiated
with the no-arg constructor, the algorithm's stopping criterion that will
matter is the "DEFAULT_ABSOLUTE_ACCURACY" and that one *is* indeed redefined
in each solver.
And again, I don't think that it is good that this default exists because it
makes it much less obvious that the criterion might in fact not satisfy the
user's actual need.
Of course, we have to balance that with ease of uses in some cases...
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Phil Steitz <ph...@gmail.com>.
On 11/6/11 8:37 AM, Sébastien Brisard wrote:
>>
>> [I did not follow all the details of this discussion; sorry if I'm
>> slightly off base.] But, if somewhere some _default_ accuracy is
>> needed to pass to a _default_ solver, I'd say: instantiate the
>> solver using its _default_ constructor; thus, no need to chase up
>> instance variables used further up the hierarchy.
>>
>>
>> Gilles
>>
> Yes, that's a good suggestion, I'll work on this idea.
Unless you are going to eliminate the bracketing step, this will not
work. I think I now remember why that code is there, as you will
likely find in researching the archives. The test
if (FastMath.abs(rootFindingFunction.value(lowerBound)) <
getSolverAbsoluteAccuracy()) {
return lowerBound;
}
and similar for the upperBound is there to avoid convergence
problems in corner cases where the inverse cum is being evaluated
near 0 or 1 and the domain upper / lower bound should be returned.
You correctly pointed out that what should be used in the test is
the function value accuracy - logically attached to the
distribution, not a solver, since if this test succeeds, no solver
will be created. I think its best to either somehow convince
ourselves that we can eliminate this test or define a default
function value accuracy threshold to use in the test.
Phil
>
> Sébastien
>
>
> ---------------------------------------------------------------------
>
>
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
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Sébastien Brisard <se...@m4x.org>.
>
> [I did not follow all the details of this discussion; sorry if I'm slightly
> off base.] But, if somewhere some _default_ accuracy is needed to pass to a
> _default_ solver, I'd say: instantiate the solver using its _default_
> constructor; thus, no need to chase up instance variables used further up
> the hierarchy.
>
>
> Gilles
>
Yes, that's a good suggestion, I'll work on this idea.
Sébastien
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Phil Steitz <ph...@gmail.com>.
On 11/6/11 10:24 AM, Gilles Sadowski wrote:
> On Sun, Nov 06, 2011 at 09:28:48AM -0700, Phil Steitz wrote:
>> On 11/6/11 7:59 AM, Gilles Sadowski wrote:
>>> On Sun, Nov 06, 2011 at 12:24:03AM -0700, Phil Steitz wrote:
>>>> On 11/6/11 12:18 AM, Sébastien Brisard wrote:
>>>>>> +1, but your observation above then leads to the question where are
>>>>>> you going to get this value from? There may not be a solver to read
>>>>>> it from. I guess the default impl in the base class could just
>>>>>> return BaseAbstractUnivariateRealSolver.DEFAULT_FUNCTION_VALUE_ACCURACY.
>>>>>>
>>>>>> Phil
>>>>>>
>>>>> Ah, that's one option I didn't think of. I intended to follow the same
>>>>> scheme as for getSolverAbsoluteAccuracy(), which returns
>>>>> solverAbsoluteAccuracy (private field), possibly initialized with
>>>>> DEFAULT_SOLVER_ABSOLUTE_ACCURACY (see AbstractContinuousDistribution).
>>>>> Actually, using the BaseAbstractUnivariateRealSolver default value is
>>>>> probably better. However, these default values are private... So maybe
>>>>> we will have to select reasonable default values here.
>>>> Ugh. I remember complaining about that (the fact that the defaults
>>>> were made private) for precisely this kind of reason. Have to clone
>>>> the value, I guess and keep it in synch with whatever Brent uses.
>>>>
>>>> Phil
>>>>> Sébastien
>>> [I did not follow all the details of this discussion; sorry if I'm slightly
>>> off base.] But, if somewhere some _default_ accuracy is needed to pass to a
>>> _default_ solver, I'd say: instantiate the solver using its _default_
>>> constructor; thus, no need to chase up instance variables used further up
>>> the hierarchy.
>> Read the post. What is being reused is the default value itself.
>> There may or may not be a solver instantiated.
> If there is no solver, why would you want to access an instance variable of
> the solvers's base class?
What the code could use is access to the *default* function value
accuracy, essentially to reuse the default value. This is a reason
that it is good to expose default configuration properties as public
constants. What the code is doing is checking to see if the domain
upper / lower bounds return values under the function that are
within the default function value accuracy (logically a property of
the distribution at this point). If this test succeeds, one of the
domain endpoints will be returned and no solver will be
instantiated. It would be nice from a consistency perspective to
use the default that BaseAbstractUnivariateRealSolver defines.
Phil
>
>
> 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
Re: [math] replace
AbstractContinuousDistribution.getSolverAbsoluteAccuracy() with
AbstractContinuousDistribution.getSolver()
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
On Sun, Nov 06, 2011 at 09:28:48AM -0700, Phil Steitz wrote:
> On 11/6/11 7:59 AM, Gilles Sadowski wrote:
> > On Sun, Nov 06, 2011 at 12:24:03AM -0700, Phil Steitz wrote:
> >> On 11/6/11 12:18 AM, Sébastien Brisard wrote:
> >>>> +1, but your observation above then leads to the question where are
> >>>> you going to get this value from? There may not be a solver to read
> >>>> it from. I guess the default impl in the base class could just
> >>>> return BaseAbstractUnivariateRealSolver.DEFAULT_FUNCTION_VALUE_ACCURACY.
> >>>>
> >>>> Phil
> >>>>
> >>> Ah, that's one option I didn't think of. I intended to follow the same
> >>> scheme as for getSolverAbsoluteAccuracy(), which returns
> >>> solverAbsoluteAccuracy (private field), possibly initialized with
> >>> DEFAULT_SOLVER_ABSOLUTE_ACCURACY (see AbstractContinuousDistribution).
> >>> Actually, using the BaseAbstractUnivariateRealSolver default value is
> >>> probably better. However, these default values are private... So maybe
> >>> we will have to select reasonable default values here.
> >> Ugh. I remember complaining about that (the fact that the defaults
> >> were made private) for precisely this kind of reason. Have to clone
> >> the value, I guess and keep it in synch with whatever Brent uses.
> >>
> >> Phil
> >>> Sébastien
> > [I did not follow all the details of this discussion; sorry if I'm slightly
> > off base.] But, if somewhere some _default_ accuracy is needed to pass to a
> > _default_ solver, I'd say: instantiate the solver using its _default_
> > constructor; thus, no need to chase up instance variables used further up
> > the hierarchy.
>
> Read the post. What is being reused is the default value itself.
> There may or may not be a solver instantiated.
If there is no solver, why would you want to access an instance variable of
the solvers's base class?
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Phil Steitz <ph...@gmail.com>.
On 11/6/11 7:59 AM, Gilles Sadowski wrote:
> On Sun, Nov 06, 2011 at 12:24:03AM -0700, Phil Steitz wrote:
>> On 11/6/11 12:18 AM, Sébastien Brisard wrote:
>>>> +1, but your observation above then leads to the question where are
>>>> you going to get this value from? There may not be a solver to read
>>>> it from. I guess the default impl in the base class could just
>>>> return BaseAbstractUnivariateRealSolver.DEFAULT_FUNCTION_VALUE_ACCURACY.
>>>>
>>>> Phil
>>>>
>>> Ah, that's one option I didn't think of. I intended to follow the same
>>> scheme as for getSolverAbsoluteAccuracy(), which returns
>>> solverAbsoluteAccuracy (private field), possibly initialized with
>>> DEFAULT_SOLVER_ABSOLUTE_ACCURACY (see AbstractContinuousDistribution).
>>> Actually, using the BaseAbstractUnivariateRealSolver default value is
>>> probably better. However, these default values are private... So maybe
>>> we will have to select reasonable default values here.
>> Ugh. I remember complaining about that (the fact that the defaults
>> were made private) for precisely this kind of reason. Have to clone
>> the value, I guess and keep it in synch with whatever Brent uses.
>>
>> Phil
>>> Sébastien
> [I did not follow all the details of this discussion; sorry if I'm slightly
> off base.] But, if somewhere some _default_ accuracy is needed to pass to a
> _default_ solver, I'd say: instantiate the solver using its _default_
> constructor; thus, no need to chase up instance variables used further up
> the hierarchy.
Read the post. What is being reused is the default value itself.
There may or may not be a solver instantiated.
Phil
>
>
> 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
Re: [math] replace
AbstractContinuousDistribution.getSolverAbsoluteAccuracy() with
AbstractContinuousDistribution.getSolver()
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
On Sun, Nov 06, 2011 at 12:24:03AM -0700, Phil Steitz wrote:
> On 11/6/11 12:18 AM, Sébastien Brisard wrote:
> >> +1, but your observation above then leads to the question where are
> >> you going to get this value from? There may not be a solver to read
> >> it from. I guess the default impl in the base class could just
> >> return BaseAbstractUnivariateRealSolver.DEFAULT_FUNCTION_VALUE_ACCURACY.
> >>
> >> Phil
> >>
> > Ah, that's one option I didn't think of. I intended to follow the same
> > scheme as for getSolverAbsoluteAccuracy(), which returns
> > solverAbsoluteAccuracy (private field), possibly initialized with
> > DEFAULT_SOLVER_ABSOLUTE_ACCURACY (see AbstractContinuousDistribution).
> > Actually, using the BaseAbstractUnivariateRealSolver default value is
> > probably better. However, these default values are private... So maybe
> > we will have to select reasonable default values here.
>
> Ugh. I remember complaining about that (the fact that the defaults
> were made private) for precisely this kind of reason. Have to clone
> the value, I guess and keep it in synch with whatever Brent uses.
>
> Phil
> > Sébastien
[I did not follow all the details of this discussion; sorry if I'm slightly
off base.] But, if somewhere some _default_ accuracy is needed to pass to a
_default_ solver, I'd say: instantiate the solver using its _default_
constructor; thus, no need to chase up instance variables used further up
the hierarchy.
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Phil Steitz <ph...@gmail.com>.
On 11/6/11 12:18 AM, Sébastien Brisard wrote:
>> +1, but your observation above then leads to the question where are
>> you going to get this value from? There may not be a solver to read
>> it from. I guess the default impl in the base class could just
>> return BaseAbstractUnivariateRealSolver.DEFAULT_FUNCTION_VALUE_ACCURACY.
>>
>> Phil
>>
> Ah, that's one option I didn't think of. I intended to follow the same
> scheme as for getSolverAbsoluteAccuracy(), which returns
> solverAbsoluteAccuracy (private field), possibly initialized with
> DEFAULT_SOLVER_ABSOLUTE_ACCURACY (see AbstractContinuousDistribution).
> Actually, using the BaseAbstractUnivariateRealSolver default value is
> probably better. However, these default values are private... So maybe
> we will have to select reasonable default values here.
Ugh. I remember complaining about that (the fact that the defaults
were made private) for precisely this kind of reason. Have to clone
the value, I guess and keep it in synch with whatever Brent uses.
Phil
> Sébastien
>
>
> ---------------------------------------------------------------------
> 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
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Sébastien Brisard <se...@m4x.org>.
>
> +1, but your observation above then leads to the question where are
> you going to get this value from? There may not be a solver to read
> it from. I guess the default impl in the base class could just
> return BaseAbstractUnivariateRealSolver.DEFAULT_FUNCTION_VALUE_ACCURACY.
>
> Phil
>
Ah, that's one option I didn't think of. I intended to follow the same
scheme as for getSolverAbsoluteAccuracy(), which returns
solverAbsoluteAccuracy (private field), possibly initialized with
DEFAULT_SOLVER_ABSOLUTE_ACCURACY (see AbstractContinuousDistribution).
Actually, using the BaseAbstractUnivariateRealSolver default value is
probably better. However, these default values are private... So maybe
we will have to select reasonable default values here.
Sébastien
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Phil Steitz <ph...@gmail.com>.
On 11/5/11 10:16 AM, Sébastien Brisard wrote:
>> We are talking about two different things here - 0) configuring the
>> solver and 1) reading its properties (in the present case, as part
>> of internal implementation).
>>
>> Regarding configuration, the current setup where distribution
>> constructors have optional inverse cum absolute accuracy arguments
>> is good and should not be changed, IMO. Users may have no idea what
>> solver to use, but a good idea how accurate they want their inverse
>> cum results to be, so we should keep that config option. Allowing a
>> fully configured solver to be passed in may be overkill, but if we
>> want to add a protected constructor in the abstract base class and
>> add implementations to all of the distributions to support it, I am
>> OK with that. Just don't drop the current simple constructors that
>> either just accept defaults fully or allow the absolute accuracy to
>> be passed in as a double.
>>
> Indeed, my idea was to keep a constructor with simple parameters for
> the configuration of the default solver (Brent, in the present
> implementation), alongside with a more versatile one.
>
> I've had second thoughts about the whole thing, and came to
> approximately the same conclusion as you. I've realized that
> sub-classes might override inverseCumulativeProbability, and in some
> cases, the new implementation of this method might do without a solver
> at all (for very simple distributions). So why provide a solver at
> construction time? In the present implementation, a new instance of
> the default solver is created at each call.
That is a good point I had not thought of. I agree that because of
this, we should not force creation of a solver in the base class.
>
>> Regarding read access to the properties of the solver, another
>> option is to just add a protected getSolver and be done with it.
>> Alternatively, getFunctionValueAbsoluteAccuracy could be exposed
>> either protected or public. The getter for absolute accuracy in the
>> distribution context really mirrors the optional constructor
>> argument above.
>>
>> I guess my recommendation is to solve the problem we actually have
>> with the simplest change, which is probably just a protected
>> accessor for the solver.
>>
> I like the least change principle! For the reason explained above (a
> solver might not always be meaningful), I would favour an accessor
> like getSolverFunctionValueAccuracy(), though, instead of getSolver().
> Although I don't really mind, since it would be protected anyway
+1, but your observation above then leads to the question where are
you going to get this value from? There may not be a solver to read
it from. I guess the default impl in the base class could just
return BaseAbstractUnivariateRealSolver.DEFAULT_FUNCTION_VALUE_ACCURACY.
Phil
> Are you OK with that?
>
> Sébastien
>> Phil
>>> Sébastien
>>>
>>>
>>> ---------------------------------------------------------------------
>>> 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
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Sébastien Brisard <se...@m4x.org>.
>
> We are talking about two different things here - 0) configuring the
> solver and 1) reading its properties (in the present case, as part
> of internal implementation).
>
> Regarding configuration, the current setup where distribution
> constructors have optional inverse cum absolute accuracy arguments
> is good and should not be changed, IMO. Users may have no idea what
> solver to use, but a good idea how accurate they want their inverse
> cum results to be, so we should keep that config option. Allowing a
> fully configured solver to be passed in may be overkill, but if we
> want to add a protected constructor in the abstract base class and
> add implementations to all of the distributions to support it, I am
> OK with that. Just don't drop the current simple constructors that
> either just accept defaults fully or allow the absolute accuracy to
> be passed in as a double.
>
Indeed, my idea was to keep a constructor with simple parameters for
the configuration of the default solver (Brent, in the present
implementation), alongside with a more versatile one.
I've had second thoughts about the whole thing, and came to
approximately the same conclusion as you. I've realized that
sub-classes might override inverseCumulativeProbability, and in some
cases, the new implementation of this method might do without a solver
at all (for very simple distributions). So why provide a solver at
construction time? In the present implementation, a new instance of
the default solver is created at each call.
>
> Regarding read access to the properties of the solver, another
> option is to just add a protected getSolver and be done with it.
> Alternatively, getFunctionValueAbsoluteAccuracy could be exposed
> either protected or public. The getter for absolute accuracy in the
> distribution context really mirrors the optional constructor
> argument above.
>
> I guess my recommendation is to solve the problem we actually have
> with the simplest change, which is probably just a protected
> accessor for the solver.
>
I like the least change principle! For the reason explained above (a
solver might not always be meaningful), I would favour an accessor
like getSolverFunctionValueAccuracy(), though, instead of getSolver().
Although I don't really mind, since it would be protected anyway.
Are you OK with that?
Sébastien
> Phil
>>
>> Sébastien
>>
>>
>> ---------------------------------------------------------------------
>> 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
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Phil Steitz <ph...@gmail.com>.
On 11/5/11 7:22 AM, Sébastien Brisard wrote:
>> So the user would first configure the solver and then provide it fully
>> configured to the abstract distribution ?
>>
>> If this is what you propose, then I agree.
>>
>> Luc
>>
> Yes, that's exactly what I had in mind.
We are talking about two different things here - 0) configuring the
solver and 1) reading its properties (in the present case, as part
of internal implementation).
Regarding configuration, the current setup where distribution
constructors have optional inverse cum absolute accuracy arguments
is good and should not be changed, IMO. Users may have no idea what
solver to use, but a good idea how accurate they want their inverse
cum results to be, so we should keep that config option. Allowing a
fully configured solver to be passed in may be overkill, but if we
want to add a protected constructor in the abstract base class and
add implementations to all of the distributions to support it, I am
OK with that. Just don't drop the current simple constructors that
either just accept defaults fully or allow the absolute accuracy to
be passed in as a double.
Regarding read access to the properties of the solver, another
option is to just add a protected getSolver and be done with it.
Alternatively, getFunctionValueAbsoluteAccuracy could be exposed
either protected or public. The getter for absolute accuracy in the
distribution context really mirrors the optional constructor
argument above.
I guess my recommendation is to solve the problem we actually have
with the simplest change, which is probably just a protected
accessor for the solver.
Phil
>
> Sébastien
>
>
> ---------------------------------------------------------------------
> 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
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Sébastien Brisard <se...@m4x.org>.
>
> So the user would first configure the solver and then provide it fully
> configured to the abstract distribution ?
>
> If this is what you propose, then I agree.
>
> Luc
>
Yes, that's exactly what I had in mind.
Sébastien
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] replace AbstractContinuousDistribution.getSolverAbsoluteAccuracy()
with AbstractContinuousDistribution.getSolver()
Posted by Luc Maisonobe <Lu...@free.fr>.
Le 05/11/2011 09:21, Sébastien Brisard a écrit :
> Hi,
> while working on MATH-699 (which was triggered by MATH-692), I found
> some inconsistencies in the use of
> UnivariateRealSolver.getAbsoluteAccuracy() and
> UnivariateRealSolver.getFunctionValueAccuracy() (see the JIRA ticket
> for more details).
> In short, the current implementation of AbstractContinuousDistribution
> allows customization of the default solver (for inverting the CDF) via
> AbstractContinuousDistribution.getSolverAbsoluteAccuracy(). To be
> completely consistent, we would also need to provide a new method
> AbstractContinuousDistribution.getSolverFunctionValueAccuracy().
> Instead, I suggest we open the possibility to provide a custom solver
> to AbstractContinuousDistribution (with all three tolerances set by
> the user), as well as a getSolver() method.
> What do you think of this change?
So the user would first configure the solver and then provide it fully
configured to the abstract distribution ?
If this is what you propose, then I agree.
Luc
>
> Sébastien
>
>
> ---------------------------------------------------------------------
> 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