You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Luc Maisonobe <Lu...@free.fr> on 2010/12/06 20:29:48 UTC
Re: svn commit: r1042596 - in /commons/proper/math/trunk/src/main/java/org/apache/commons/math:
analysis/solvers/ distribution/
Hi,
Le 06/12/2010 12:56, erans@apache.org a écrit :
> Author: erans
> Date: Mon Dec 6 11:56:26 2010
> New Revision: 1042596
>
> URL: http://svn.apache.org/viewvc?rev=1042596&view=rev
> Log:
> MATH-452
> Made all static variables (accuracies) "private".
We have already started a discussion on a similar point some months ago.
Last message is here: <http://markmail.org/message/m4l7v4pdfp5yn3as>,
but we did not really conclude.
To summarize my point from a few months ago, one use case for these
public values is that a use program can do something like this to first
initialize a Graphical User Interface, then let the user change the
values he wants, and later retrieve the actual values (which may be
unchanged) to build the solver.
// initialize GUI
solverGui.setRelAccuracy(BaseAbstractUnivariateRealSolver.DEFAULT_RELATIVE_ACCURACY);
solverGui.setAbsAccuracy(BaseAbstractUnivariateRealSolver.DEFAULT_ABSOLUTE_ACCURACY);
// fire the GUI
...
// in the action callback from the GUI OK button, create the solver
double rel = solverGui.getRelAccuracy();
double abs = solverGui.getAbsAccuracy();
BrentSolver mySolver = new BrentSolver(rel, abs);
Another simpler use case is simply to have the default value available
in the Javadoc without being forced to get commons-math sources.
So I still consider this kind of public constats are usefule. Despite
the fact within commons-math they are used only in some constructors,
the fact they are public allow them to be also used in user code.
Luc
>
> Modified:
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BaseAbstractUnivariateRealSolver.java
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BisectionSolver.java
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BrentSolver.java
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/LaguerreSolver.java
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/MullerSolver.java
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/MullerSolver2.java
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/NewtonSolver.java
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/RiddersSolver.java
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/SecantSolver.java
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/distribution/AbstractContinuousDistribution.java
>
> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BaseAbstractUnivariateRealSolver.java
> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BaseAbstractUnivariateRealSolver.java?rev=1042596&r1=1042595&r2=1042596&view=diff
> ==============================================================================
> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BaseAbstractUnivariateRealSolver.java (original)
> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BaseAbstractUnivariateRealSolver.java Mon Dec 6 11:56:26 2010
> @@ -34,12 +34,10 @@ import org.apache.commons.math.analysis.
> */
> public abstract class BaseAbstractUnivariateRealSolver<FUNC extends UnivariateRealFunction>
> implements BaseUnivariateRealSolver<FUNC> {
> - /** Default absolute accuracy */
> - public static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
> /** Default relative accuracy. */
> - public static final double DEFAULT_RELATIVE_ACCURACY = 1e-14;
> + private static final double DEFAULT_RELATIVE_ACCURACY = 1e-14;
> /** Default function value accuracy. */
> - public static final double DEFAULT_FUNCTION_VALUE_ACCURACY = 1e-15;
> + private static final double DEFAULT_FUNCTION_VALUE_ACCURACY = 1e-15;
> /** Function value accuracy. */
> private final double functionValueAccuracy;
> /** Absolute accuracy. */
>
> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BisectionSolver.java
> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BisectionSolver.java?rev=1042596&r1=1042595&r2=1042596&view=diff
> ==============================================================================
> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BisectionSolver.java (original)
> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BisectionSolver.java Mon Dec 6 11:56:26 2010
> @@ -28,10 +28,10 @@ import org.apache.commons.math.util.Fast
> */
> public class BisectionSolver extends AbstractUnivariateRealSolver {
> /** Default absolute accuracy. */
> - public static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
> + private static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
>
> /**
> - * Construct a solver with default accuracy.
> + * Construct a solver with default accuracy (1e-6).
> */
> public BisectionSolver() {
> this(DEFAULT_ABSOLUTE_ACCURACY);
>
> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BrentSolver.java
> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BrentSolver.java?rev=1042596&r1=1042595&r2=1042596&view=diff
> ==============================================================================
> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BrentSolver.java (original)
> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/BrentSolver.java Mon Dec 6 11:56:26 2010
> @@ -37,10 +37,10 @@ public class BrentSolver extends Abstrac
> /** Serializable version identifier */
> private static final long serialVersionUID = 7694577816772532779L;
> /** Default absolute accuracy. */
> - public static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
> + private static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
>
> /**
> - * Construct a solver with default accuracies.
> + * Construct a solver with default accuracy (1e-6).
> */
> public BrentSolver() {
> this(DEFAULT_ABSOLUTE_ACCURACY);
>
> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/LaguerreSolver.java
> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/LaguerreSolver.java?rev=1042596&r1=1042595&r2=1042596&view=diff
> ==============================================================================
> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/LaguerreSolver.java (original)
> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/LaguerreSolver.java Mon Dec 6 11:56:26 2010
> @@ -40,12 +40,12 @@ import org.apache.commons.math.util.Fast
> */
> public class LaguerreSolver extends AbstractPolynomialSolver {
> /** Default absolute accuracy. */
> - public static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
> + private static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
> /** Complex solver. */
> protected ComplexSolver complexSolver = new ComplexSolver();
>
> /**
> - * Construct a solver with default accuracies.
> + * Construct a solver with default accuracy (1e-6).
> */
> public LaguerreSolver() {
> this(DEFAULT_ABSOLUTE_ACCURACY);
>
> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/MullerSolver.java
> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/MullerSolver.java?rev=1042596&r1=1042595&r2=1042596&view=diff
> ==============================================================================
> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/MullerSolver.java (original)
> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/MullerSolver.java Mon Dec 6 11:56:26 2010
> @@ -49,10 +49,10 @@ public class MullerSolver extends Abstra
> /** Serializable version identifier */
> private static final long serialVersionUID = 7694577816772532779L;
> /** Default absolute accuracy. */
> - public static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
> + private static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
>
> /**
> - * Construct a solver with default accuracies.
> + * Construct a solver with default accuracy (1e-6).
> */
> public MullerSolver() {
> this(DEFAULT_ABSOLUTE_ACCURACY);
>
> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/MullerSolver2.java
> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/MullerSolver2.java?rev=1042596&r1=1042595&r2=1042596&view=diff
> ==============================================================================
> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/MullerSolver2.java (original)
> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/MullerSolver2.java Mon Dec 6 11:56:26 2010
> @@ -49,10 +49,10 @@ public class MullerSolver2 extends Abstr
> /** Serializable version identifier */
> private static final long serialVersionUID = 7694577816772532779L;
> /** Default absolute accuracy. */
> - public static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
> + private static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
>
> /**
> - * Construct a solver with default accuracies.
> + * Construct a solver with default accuracy (1e-6).
> */
> public MullerSolver2() {
> this(DEFAULT_ABSOLUTE_ACCURACY);
>
> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/NewtonSolver.java
> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/NewtonSolver.java?rev=1042596&r1=1042595&r2=1042596&view=diff
> ==============================================================================
> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/NewtonSolver.java (original)
> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/NewtonSolver.java Mon Dec 6 11:56:26 2010
> @@ -30,7 +30,7 @@ import org.apache.commons.math.util.Fast
> */
> public class NewtonSolver extends AbstractDifferentiableUnivariateRealSolver {
> /** Default absolute accuracy. */
> - public static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
> + private static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
>
> /**
> * Construct a solver.
>
> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/RiddersSolver.java
> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/RiddersSolver.java?rev=1042596&r1=1042595&r2=1042596&view=diff
> ==============================================================================
> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/RiddersSolver.java (original)
> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/RiddersSolver.java Mon Dec 6 11:56:26 2010
> @@ -33,10 +33,10 @@ import org.apache.commons.math.util.Math
> */
> public class RiddersSolver extends AbstractUnivariateRealSolver {
> /** Default absolute accuracy. */
> - public static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
> + private static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
>
> /**
> - * Construct a solver with default accuracy.
> + * Construct a solver with default accuracy (1e-6).
> */
> public RiddersSolver() {
> this(DEFAULT_ABSOLUTE_ACCURACY);
>
> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/SecantSolver.java
> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/SecantSolver.java?rev=1042596&r1=1042595&r2=1042596&view=diff
> ==============================================================================
> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/SecantSolver.java (original)
> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/analysis/solvers/SecantSolver.java Mon Dec 6 11:56:26 2010
> @@ -38,10 +38,10 @@ import org.apache.commons.math.util.Fast
> */
> public class SecantSolver extends AbstractUnivariateRealSolver {
> /** Default absolute accuracy. */
> - public static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
> + private static final double DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
>
> /**
> - * Construct a solver with default accuracy.
> + * Construct a solver with default accuracy (1e-6).
> */
> public SecantSolver() {
> this(DEFAULT_ABSOLUTE_ACCURACY);
>
> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/distribution/AbstractContinuousDistribution.java
> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/distribution/AbstractContinuousDistribution.java?rev=1042596&r1=1042595&r2=1042596&view=diff
> ==============================================================================
> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/distribution/AbstractContinuousDistribution.java (original)
> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/distribution/AbstractContinuousDistribution.java Mon Dec 6 11:56:26 2010
> @@ -42,16 +42,18 @@ public abstract class AbstractContinuous
> implements ContinuousDistribution, Serializable {
> /** Serializable version identifier */
> private static final long serialVersionUID = -38038050983108802L;
> + /** Default accuracy. */
> + public static final double SOLVER_DEFAULT_ABSOLUTE_ACCURACY = 1e-6;
> /**
> * RandomData instance used to generate samples from the distribution
> * @since 2.2
> */
> protected final RandomDataImpl randomData = new RandomDataImpl();
> /**
> - * Solver absolute accuracy for inverse cumulative computation
> + * Solver absolute accuracy for inverse cumulative computation.
> * @since 2.1
> */
> - private double solverAbsoluteAccuracy = BrentSolver.DEFAULT_ABSOLUTE_ACCURACY;
> + private double solverAbsoluteAccuracy = SOLVER_DEFAULT_ABSOLUTE_ACCURACY;
> /**
> * Default constructor.
> */
> @@ -125,7 +127,7 @@ public abstract class AbstractContinuous
> // find root
> double root = UnivariateRealSolverUtils.solve(rootFindingFunction,
> // override getSolverAbsoluteAccuracy() to use a Brent solver with
> - // absolute accuracy different from BrentSolver default
> + // absolute accuracy different from the default.
> bracket[0],bracket[1], getSolverAbsoluteAccuracy());
> return root;
> }
> @@ -209,6 +211,8 @@ public abstract class AbstractContinuous
>
> /**
> * Returns the solver absolute accuracy for inverse cumulative computation.
> + * You can override this method in order to use a Brent solver with an
> + * absolute accuracy different from the default.
> *
> * @return the maximum absolute error in inverse cumulative probability estimates
> * @since 2.1
> @@ -216,5 +220,4 @@ public abstract class AbstractContinuous
> protected double getSolverAbsoluteAccuracy() {
> return solverAbsoluteAccuracy;
> }
> -
> }
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1042596 - in
/commons/proper/math/trunk/src/main/java/org/apache/commons/math:
analysis/solvers/ distribution/
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
Hi.
> [...]
> >>> ---CUT---
> >>>> BrentSolver mySolver = new BrentSolver();
> >>>>
> >>>> // initialize GUI
> >>>> solverGui.setRelAccuracy(mySolver.getRelativeAccuracy());
> >>>> solverGui.setAbsAccuracy(mySolver.getAbsoluteAccuracy());
> >>>>
> >>>> // fire the GUI
> >>>> ...
> >>>>
> >>>> // in the action callback from the GUI OK button, create the solver
> >>>> double rel = solverGui.getRelAccuracy();
> >>>> double abs = solverGui.getAbsAccuracy();
> >>>>
> >>>> mySolver = new BrentSolver(rel, abs);
> >>> ---CUT---
> >>
> >> Creating two solvers to use only one and simply hiding constants seems
> >> awkward to me. Just letting the constants visible is simpler.
> >
> > You don't use only one. And, arguably, for the GUI, the first solver
> > instance is the most important because it provides the needed information.
> >
> > What bothers me most with the variables is:
> > 1. The redundancy:
> > The value is stored once in the static variable of the class and a
> > second time in each instance.
>
> I don't understand. This is exactly what you have already done. The
> current implementation only changed the static variables from public to
> private. There are still independent class variables in each class and
> independent instant variables in each instance (inherited from the
> abstract class).
Well that's true. But a "private" named constant is only used internally (to
make the code clearer), and we can assume that the coder of that class will
indeed use the right constant in order to initialize that class; that's not
really a problem.
What I don't like is that the constant is defined in another (parent) class
and that it is public; the value can then accessed in two ways: the static
field and the setter. That's what I meant by "redundant".
> > 2. The possible inconsistency:
> > Nothing prevents a (sub)class to *not* use the default. So the
> > documentation could be misleading whereas the accessor will always
> > provide the actual value.
>
> Of course a subclass can use something else, this is a main feature of
> subclassing: adapting behavior. Also regardless of how the default value
> is defined (once publicly in a top level class or several time privately
> in each subclass), the user can always choose to build his builder with
> non default values.
And that will lead to the inconsistency problem below...
> The accessors are not here to provide defaults, they
> are here to provide the actual values.
Yes. That's exactly what I said; the GUI (or whatever use-case) should be
sure to get the actual default value of the specific class at hand, not a
possible default suggested by a base class.
> What we are discussing here is not about the actual values that are
> stored in the instances. We are discussing about the default constant
> values which *may* be used to initialize the actual ones. They are
> simple constants, they are currently all the same and they are
> duplicated 8 times.
Of course, that's not pretty; but as a matter of principle, these constants
might not have the same meaning (although they can have the same value).
What I stress is that they should be tied to the specific class, not to the
base class.
> >
> > GUIs _are_ awkward. Having to create two solvers in such a context is
> > negligible and a very small price to pay for ensuring consistency.
> >
> >> I think what Sebb pointed out in this issue was not that the constants
> >> were public, but that these constants were duplicated. I agree with him
> >> here. However solving the issue by removing the duplication and having
> >> constants defined in the top level interface is more straightforward.
> >
> > There are 2 problems with this:
> > 1. The top-level class where such "public" constants should be defined is
> > "BaseAbstractUnivariateRealSolver". This is not a "user" class but a
> > "developer" class, an implementation detail. Encouraging users to have
> > that class appear in their code is not good design.
>
> Then we can put them in the UnivariateRealSolver interface. Interfaces
> can have constants and are a great place to store them in fact.
The above problem of principle is the same; I think that it is not
appropriate to define a variable without knowing what it actually means.
What would be the comment associated of, say, DEFAULT_ABSOLUTE_ACCURACY.
Whatever you decide here can be wrong in some implementations. Hence it is
wrong at the interface level.
> > 2. The "accuracies" might mean different things for different solvers and
> > thus the defaults might be set to different values depending on the
> > implementation. [That is why I had created a static variable for each
> > class.]
>
> If a specific implementation needs a different value, then it can use
> its own default, either by having its own constant (it cannot reuse the
> name from the top interface here). However, for now it is clearly not
> the case. There are 8 implementations and 8 times the exact same value
> (1.0e-6).
As said above, the numeric value is not the problem.
> >
> > Please note also that by using the accessor, users can code through
> > interface while with the static constant, their code must reference a
> > concrete class.
>
> No, an interface can define a constant.
I didn't know, but that doesn't change my opinion... :-}
IMO, if some constructor uses default values for some parameters, they
should be documented at that point, ideally with the reason why this a
good default or how the user can choose some "good" value. [See e.g. the
comment for the constructor of "BrentOptimizer", which I've reproduced from
Brent's book.]
I fear that anything else can be either misleading or downright wrong.
I just advocate for staying on the safe side.
Regards,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1042596 - in /commons/proper/math/trunk/src/main/java/org/apache/commons/math:
analysis/solvers/ distribution/
Posted by Luc Maisonobe <Lu...@free.fr>.
Hi Gilles,
Le 07/12/2010 10:47, Gilles Sadowski a écrit :
> Hello.
>
>>> [...]
>>>
>>> I think that we can satisfy this use-case without exposing the variables:
>>>
>>> ---CUT---
>>>> BrentSolver mySolver = new BrentSolver();
>>>>
>>>> // initialize GUI
>>>> solverGui.setRelAccuracy(mySolver.getRelativeAccuracy());
>>>> solverGui.setAbsAccuracy(mySolver.getAbsoluteAccuracy());
>>>>
>>>> // fire the GUI
>>>> ...
>>>>
>>>> // in the action callback from the GUI OK button, create the solver
>>>> double rel = solverGui.getRelAccuracy();
>>>> double abs = solverGui.getAbsAccuracy();
>>>>
>>>> mySolver = new BrentSolver(rel, abs);
>>> ---CUT---
>>
>> Creating two solvers to use only one and simply hiding constants seems
>> awkward to me. Just letting the constants visible is simpler.
>
> You don't use only one. And, arguably, for the GUI, the first solver
> instance is the most important because it provides the needed information.
>
> What bothers me most with the variables is:
> 1. The redundancy:
> The value is stored once in the static variable of the class and a
> second time in each instance.
I don't understand. This is exactly what you have already done. The
current implementation only changed the static variables from public to
private. There are still independent class variables in each class and
independent instant variables in each instance (inherited from the
abstract class).
> 2. The possible inconsistency:
> Nothing prevents a (sub)class to *not* use the default. So the
> documentation could be misleading whereas the accessor will always
> provide the actual value.
Of course a subclass can use something else, this is a main feature of
subclassing: adapting behavior. Also regardless of how the default value
is defined (once publicly in a top level class or several time privately
in each subclass), the user can always choose to build his builder with
non default values. The accessors are not here to provide defaults, they
are here to provide the actual values.
What we are discussing here is not about the actual values that are
stored in the instances. We are discussing about the default constant
values which *may* be used to initialize the actual ones. They are
simple constants, they are currently all the same and they are
duplicated 8 times.
>
> GUIs _are_ awkward. Having to create two solvers in such a context is
> negligible and a very small price to pay for ensuring consistency.
>
>> I think what Sebb pointed out in this issue was not that the constants
>> were public, but that these constants were duplicated. I agree with him
>> here. However solving the issue by removing the duplication and having
>> constants defined in the top level interface is more straightforward.
>
> There are 2 problems with this:
> 1. The top-level class where such "public" constants should be defined is
> "BaseAbstractUnivariateRealSolver". This is not a "user" class but a
> "developer" class, an implementation detail. Encouraging users to have
> that class appear in their code is not good design.
Then we can put them in the UnivariateRealSolver interface. Interfaces
can have constants and are a great place to store them in fact.
> 2. The "accuracies" might mean different things for different solvers and
> thus the defaults might be set to different values depending on the
> implementation. [That is why I had created a static variable for each
> class.]
If a specific implementation needs a different value, then it can use
its own default, either by having its own constant (it cannot reuse the
name from the top interface here). However, for now it is clearly not
the case. There are 8 implementations and 8 times the exact same value
(1.0e-6).
>
> Please note also that by using the accessor, users can code through
> interface while with the static constant, their code must reference a
> concrete class.
No, an interface can define a constant.
best regards,
Luc
>
>
> Best,
> 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: svn commit: r1042596 - in
/commons/proper/math/trunk/src/main/java/org/apache/commons/math:
analysis/solvers/ distribution/
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
Hello.
> > [...]
> >
> > I think that we can satisfy this use-case without exposing the variables:
> >
> > ---CUT---
> >> BrentSolver mySolver = new BrentSolver();
> >>
> >> // initialize GUI
> >> solverGui.setRelAccuracy(mySolver.getRelativeAccuracy());
> >> solverGui.setAbsAccuracy(mySolver.getAbsoluteAccuracy());
> >>
> >> // fire the GUI
> >> ...
> >>
> >> // in the action callback from the GUI OK button, create the solver
> >> double rel = solverGui.getRelAccuracy();
> >> double abs = solverGui.getAbsAccuracy();
> >>
> >> mySolver = new BrentSolver(rel, abs);
> > ---CUT---
>
> Creating two solvers to use only one and simply hiding constants seems
> awkward to me. Just letting the constants visible is simpler.
You don't use only one. And, arguably, for the GUI, the first solver
instance is the most important because it provides the needed information.
What bothers me most with the variables is:
1. The redundancy:
The value is stored once in the static variable of the class and a
second time in each instance.
2. The possible inconsistency:
Nothing prevents a (sub)class to *not* use the default. So the
documentation could be misleading whereas the accessor will always
provide the actual value.
GUIs _are_ awkward. Having to create two solvers in such a context is
negligible and a very small price to pay for ensuring consistency.
> I think what Sebb pointed out in this issue was not that the constants
> were public, but that these constants were duplicated. I agree with him
> here. However solving the issue by removing the duplication and having
> constants defined in the top level interface is more straightforward.
There are 2 problems with this:
1. The top-level class where such "public" constants should be defined is
"BaseAbstractUnivariateRealSolver". This is not a "user" class but a
"developer" class, an implementation detail. Encouraging users to have
that class appear in their code is not good design.
2. The "accuracies" might mean different things for different solvers and
thus the defaults might be set to different values depending on the
implementation. [That is why I had created a static variable for each
class.]
Please note also that by using the accessor, users can code through
interface while with the static constant, their code must reference a
concrete class.
Best,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1042596 - in /commons/proper/math/trunk/src/main/java/org/apache/commons/math:
analysis/solvers/ distribution/
Posted by Phil Steitz <ph...@gmail.com>.
On Mon, Dec 6, 2010 at 5:35 PM, Luc Maisonobe <Lu...@free.fr> wrote:
> Hi Gilles,
>
> Le 06/12/2010 23:27, Gilles Sadowski a écrit :
> > Hi.
> >
> >>> Log:
> >>> MATH-452
> >>> Made all static variables (accuracies) "private".
> >>
> >>
> >> We have already started a discussion on a similar point some months ago.
> >> Last message is here: <http://markmail.org/message/m4l7v4pdfp5yn3as>,
> >> but we did not really conclude.
> >>
> >> To summarize my point from a few months ago, one use case for these
> >> public values is that a use program can do something like this to first
> >> initialize a Graphical User Interface, then let the user change the
> >> values he wants, and later retrieve the actual values (which may be
> >> unchanged) to build the solver.
> >>
> >> // initialize GUI
> >>
> solverGui.setRelAccuracy(BaseAbstractUnivariateRealSolver.DEFAULT_RELATIVE_ACCURACY);
> >>
> solverGui.setAbsAccuracy(BaseAbstractUnivariateRealSolver.DEFAULT_ABSOLUTE_ACCURACY);
> >>
> >> // fire the GUI
> >> ...
> >>
> >> // in the action callback from the GUI OK button, create the solver
> >> double rel = solverGui.getRelAccuracy();
> >> double abs = solverGui.getAbsAccuracy();
> >> BrentSolver mySolver = new BrentSolver(rel, abs);
> >>
> >>
> >> Another simpler use case is simply to have the default value available
> >> in the Javadoc without being forced to get commons-math sources.
> >>
> >> So I still consider this kind of public constats are usefule. Despite
> >> the fact within commons-math they are used only in some constructors,
> >> the fact they are public allow them to be also used in user code.
> >
> > I think that we can satisfy this use-case without exposing the variables:
> >
> > ---CUT---
> >> BrentSolver mySolver = new BrentSolver();
> >>
> >> // initialize GUI
> >> solverGui.setRelAccuracy(mySolver.getRelativeAccuracy());
> >> solverGui.setAbsAccuracy(mySolver.getAbsoluteAccuracy());
> >>
> >> // fire the GUI
> >> ...
> >>
> >> // in the action callback from the GUI OK button, create the solver
> >> double rel = solverGui.getRelAccuracy();
> >> double abs = solverGui.getAbsAccuracy();
> >>
> >> mySolver = new BrentSolver(rel, abs);
> > ---CUT---
>
> Creating two solvers to use only one and simply hiding constants seems
> awkward to me. Just letting the constants visible is simpler.
>
> I think what Sebb pointed out in this issue was not that the constants
> were public, but that these constants were duplicated. I agree with him
> here. However solving the issue by removing the duplication and having
> constants defined in the top level interface is more straightforward.
>
+1. One more thing that Gilles was careful to maintain, but gives more
reason to keep the constants public is that they are part of the
documentation - e.g.
- * Construct a solver with default accuracy.
+ * Construct a solver with default accuracy (1e-6).
Keeping the constants public eliminates the need to repeat this.
Phil
>
> best regards,
> Luc
>
> >
> >
> > Regards,
> > 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: svn commit: r1042596 - in /commons/proper/math/trunk/src/main/java/org/apache/commons/math:
analysis/solvers/ distribution/
Posted by Luc Maisonobe <Lu...@free.fr>.
Hi Gilles,
Le 06/12/2010 23:27, Gilles Sadowski a écrit :
> Hi.
>
>>> Log:
>>> MATH-452
>>> Made all static variables (accuracies) "private".
>>
>>
>> We have already started a discussion on a similar point some months ago.
>> Last message is here: <http://markmail.org/message/m4l7v4pdfp5yn3as>,
>> but we did not really conclude.
>>
>> To summarize my point from a few months ago, one use case for these
>> public values is that a use program can do something like this to first
>> initialize a Graphical User Interface, then let the user change the
>> values he wants, and later retrieve the actual values (which may be
>> unchanged) to build the solver.
>>
>> // initialize GUI
>> solverGui.setRelAccuracy(BaseAbstractUnivariateRealSolver.DEFAULT_RELATIVE_ACCURACY);
>> solverGui.setAbsAccuracy(BaseAbstractUnivariateRealSolver.DEFAULT_ABSOLUTE_ACCURACY);
>>
>> // fire the GUI
>> ...
>>
>> // in the action callback from the GUI OK button, create the solver
>> double rel = solverGui.getRelAccuracy();
>> double abs = solverGui.getAbsAccuracy();
>> BrentSolver mySolver = new BrentSolver(rel, abs);
>>
>>
>> Another simpler use case is simply to have the default value available
>> in the Javadoc without being forced to get commons-math sources.
>>
>> So I still consider this kind of public constats are usefule. Despite
>> the fact within commons-math they are used only in some constructors,
>> the fact they are public allow them to be also used in user code.
>
> I think that we can satisfy this use-case without exposing the variables:
>
> ---CUT---
>> BrentSolver mySolver = new BrentSolver();
>>
>> // initialize GUI
>> solverGui.setRelAccuracy(mySolver.getRelativeAccuracy());
>> solverGui.setAbsAccuracy(mySolver.getAbsoluteAccuracy());
>>
>> // fire the GUI
>> ...
>>
>> // in the action callback from the GUI OK button, create the solver
>> double rel = solverGui.getRelAccuracy();
>> double abs = solverGui.getAbsAccuracy();
>>
>> mySolver = new BrentSolver(rel, abs);
> ---CUT---
Creating two solvers to use only one and simply hiding constants seems
awkward to me. Just letting the constants visible is simpler.
I think what Sebb pointed out in this issue was not that the constants
were public, but that these constants were duplicated. I agree with him
here. However solving the issue by removing the duplication and having
constants defined in the top level interface is more straightforward.
best regards,
Luc
>
>
> Regards,
> 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: svn commit: r1042596 - in
/commons/proper/math/trunk/src/main/java/org/apache/commons/math:
analysis/solvers/ distribution/
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
Hi.
> > Log:
> > MATH-452
> > Made all static variables (accuracies) "private".
>
>
> We have already started a discussion on a similar point some months ago.
> Last message is here: <http://markmail.org/message/m4l7v4pdfp5yn3as>,
> but we did not really conclude.
>
> To summarize my point from a few months ago, one use case for these
> public values is that a use program can do something like this to first
> initialize a Graphical User Interface, then let the user change the
> values he wants, and later retrieve the actual values (which may be
> unchanged) to build the solver.
>
> // initialize GUI
> solverGui.setRelAccuracy(BaseAbstractUnivariateRealSolver.DEFAULT_RELATIVE_ACCURACY);
> solverGui.setAbsAccuracy(BaseAbstractUnivariateRealSolver.DEFAULT_ABSOLUTE_ACCURACY);
>
> // fire the GUI
> ...
>
> // in the action callback from the GUI OK button, create the solver
> double rel = solverGui.getRelAccuracy();
> double abs = solverGui.getAbsAccuracy();
> BrentSolver mySolver = new BrentSolver(rel, abs);
>
>
> Another simpler use case is simply to have the default value available
> in the Javadoc without being forced to get commons-math sources.
>
> So I still consider this kind of public constats are usefule. Despite
> the fact within commons-math they are used only in some constructors,
> the fact they are public allow them to be also used in user code.
I think that we can satisfy this use-case without exposing the variables:
---CUT---
> BrentSolver mySolver = new BrentSolver();
>
> // initialize GUI
> solverGui.setRelAccuracy(mySolver.getRelativeAccuracy());
> solverGui.setAbsAccuracy(mySolver.getAbsoluteAccuracy());
>
> // fire the GUI
> ...
>
> // in the action callback from the GUI OK button, create the solver
> double rel = solverGui.getRelAccuracy();
> double abs = solverGui.getAbsAccuracy();
>
> mySolver = new BrentSolver(rel, abs);
---CUT---
Regards,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org