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 2011/03/27 18:47:48 UTC
[math] cleaning code
Hi all,
I have squashed a number of findbugs and checkstyle warnings introduced
by recent changes (more than one hundred).
There are a few remaining bugs for which I would like some ideas.
Checkstyle errors:
In class MannWhitneyUTestImpl, the javadoc for private method
calculateAsymptoticPValue has a N parameter in the Javadoc which is
wrong and two n1 and n2 parameters in the signature that are not
documented. I don't know the exact meaning of n1 and n2, could someone
fix this javadoc ?
In class MathUtils, method round(double x, int scale, int
roundingMethod) we catch RuntimeException to wrap it into
MathRuntimeException. I think we should not and should simply let the
RuntimeException go up. What do you think ?
Findbugs errors:
In CMAESOptimizer constructor, we directly store references to the
inputSigma and boundaries parameters in internal fields, thus exposing
internal representation. I think we should either clone the arrays or
document the fact we will reference user arrays and set up a findbug
exclude filter. What do you think ?
In CMAESOptimizer DoubleIndex class, there is a compareTo method but no
equals method (and if we add one, there will be no hashCode method),
should we add them ?
SerializablePair extends Pair which is not serializable and does not
have an accessible void constructor. Should we add such a constructor
(perhaps setting the two fields to null), should we have
SerializablePair not extend Pair or are we sure this does work correctly
and we should add a findbugs excude filter ?
best regards,
Luc
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] cleaning code
Posted by Luc Maisonobe <Lu...@free.fr>.
Le 28/03/2011 07:25, Phil Steitz a écrit :
> On 3/27/11 6:31 PM, Phil Steitz wrote:
>> On 3/27/11 9:47 AM, Luc Maisonobe wrote:
>>> Hi all,
>>>
>>> I have squashed a number of findbugs and checkstyle warnings introduced
>>> by recent changes (more than one hundred).
>> Thanks! And sorry...
>>> There are a few remaining bugs for which I would like some ideas.
>>>
>>> Checkstyle errors:
>>>
>>> In class MannWhitneyUTestImpl, the javadoc for private method
>>> calculateAsymptoticPValue has a N parameter in the Javadoc which is
>>> wrong and two n1 and n2 parameters in the signature that are not
>>> documented. I don't know the exact meaning of n1 and n2, could someone
>>> fix this javadoc ?
>>>
>>> In class MathUtils, method round(double x, int scale, int
>>> roundingMethod) we catch RuntimeException to wrap it into
>>> MathRuntimeException. I think we should not and should simply let the
>>> RuntimeException go up. What do you think ?
>> +1 - but lets doc which RTEs will be thrown and why. Looks to me
>> like these will come from BigDecimal.setScale() which can throw
>> ArithmeticException and IllegalArgumentException, both for reasons
>> that make sense in round(...) activation context.
> I will take care of this one.
>>> Findbugs errors:
>>>
>>> In CMAESOptimizer constructor, we directly store references to the
>>> inputSigma and boundaries parameters in internal fields, thus exposing
>>> internal representation. I think we should either clone the arrays or
>>> document the fact we will reference user arrays and set up a findbug
>>> exclude filter. What do you think ?
>> Unless these things are / can be big, we should clone.
> I can also do this, if others agree.
I also think it would improve robustness. What do the CMAES optimizer
specialists think ?
Luc
>>> In CMAESOptimizer DoubleIndex class, there is a compareTo method but no
>>> equals method (and if we add one, there will be no hashCode method),
>>> should we add them ?
>> Yes.
> And this.
>
> Phil
>>> SerializablePair extends Pair which is not serializable and does not
>>> have an accessible void constructor. Should we add such a constructor
>>> (perhaps setting the two fields to null), should we have
>>> SerializablePair not extend Pair or are we sure this does work correctly
>>> and we should add a findbugs excude filter ?
>>>
>> Looks like Gilles fixed this. Thanks, Gilles!
>>
>> Phil
>>> best regards,
>>> Luc
>>>
>>> ---------------------------------------------------------------------
>>> 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] cleaning code
Posted by Phil Steitz <ph...@gmail.com>.
On 3/27/11 6:31 PM, Phil Steitz wrote:
> On 3/27/11 9:47 AM, Luc Maisonobe wrote:
>> Hi all,
>>
>> I have squashed a number of findbugs and checkstyle warnings introduced
>> by recent changes (more than one hundred).
> Thanks! And sorry...
>> There are a few remaining bugs for which I would like some ideas.
>>
>> Checkstyle errors:
>>
>> In class MannWhitneyUTestImpl, the javadoc for private method
>> calculateAsymptoticPValue has a N parameter in the Javadoc which is
>> wrong and two n1 and n2 parameters in the signature that are not
>> documented. I don't know the exact meaning of n1 and n2, could someone
>> fix this javadoc ?
>>
>> In class MathUtils, method round(double x, int scale, int
>> roundingMethod) we catch RuntimeException to wrap it into
>> MathRuntimeException. I think we should not and should simply let the
>> RuntimeException go up. What do you think ?
> +1 - but lets doc which RTEs will be thrown and why. Looks to me
> like these will come from BigDecimal.setScale() which can throw
> ArithmeticException and IllegalArgumentException, both for reasons
> that make sense in round(...) activation context.
I will take care of this one.
>> Findbugs errors:
>>
>> In CMAESOptimizer constructor, we directly store references to the
>> inputSigma and boundaries parameters in internal fields, thus exposing
>> internal representation. I think we should either clone the arrays or
>> document the fact we will reference user arrays and set up a findbug
>> exclude filter. What do you think ?
> Unless these things are / can be big, we should clone.
I can also do this, if others agree.
>> In CMAESOptimizer DoubleIndex class, there is a compareTo method but no
>> equals method (and if we add one, there will be no hashCode method),
>> should we add them ?
> Yes.
And this.
Phil
>> SerializablePair extends Pair which is not serializable and does not
>> have an accessible void constructor. Should we add such a constructor
>> (perhaps setting the two fields to null), should we have
>> SerializablePair not extend Pair or are we sure this does work correctly
>> and we should add a findbugs excude filter ?
>>
> Looks like Gilles fixed this. Thanks, Gilles!
>
> Phil
>> best regards,
>> Luc
>>
>> ---------------------------------------------------------------------
>> 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] cleaning code
Posted by Phil Steitz <ph...@gmail.com>.
On 3/27/11 9:47 AM, Luc Maisonobe wrote:
> Hi all,
>
> I have squashed a number of findbugs and checkstyle warnings introduced
> by recent changes (more than one hundred).
Thanks! And sorry...
> There are a few remaining bugs for which I would like some ideas.
>
> Checkstyle errors:
>
> In class MannWhitneyUTestImpl, the javadoc for private method
> calculateAsymptoticPValue has a N parameter in the Javadoc which is
> wrong and two n1 and n2 parameters in the signature that are not
> documented. I don't know the exact meaning of n1 and n2, could someone
> fix this javadoc ?
>
> In class MathUtils, method round(double x, int scale, int
> roundingMethod) we catch RuntimeException to wrap it into
> MathRuntimeException. I think we should not and should simply let the
> RuntimeException go up. What do you think ?
+1 - but lets doc which RTEs will be thrown and why. Looks to me
like these will come from BigDecimal.setScale() which can throw
ArithmeticException and IllegalArgumentException, both for reasons
that make sense in round(...) activation context.
> Findbugs errors:
>
> In CMAESOptimizer constructor, we directly store references to the
> inputSigma and boundaries parameters in internal fields, thus exposing
> internal representation. I think we should either clone the arrays or
> document the fact we will reference user arrays and set up a findbug
> exclude filter. What do you think ?
Unless these things are / can be big, we should clone.
> In CMAESOptimizer DoubleIndex class, there is a compareTo method but no
> equals method (and if we add one, there will be no hashCode method),
> should we add them ?
Yes.
> SerializablePair extends Pair which is not serializable and does not
> have an accessible void constructor. Should we add such a constructor
> (perhaps setting the two fields to null), should we have
> SerializablePair not extend Pair or are we sure this does work correctly
> and we should add a findbugs excude filter ?
>
Looks like Gilles fixed this. Thanks, Gilles!
Phil
> best regards,
> Luc
>
> ---------------------------------------------------------------------
> 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] cleaning code
Posted by Mikkel Meyer Andersen <mi...@mikl.dk>.
Hi Luc,
Thanks! I'll fix MannWhitneyUTestImpl.
Cheers, Mikkel.
Den 27/03/2011 18.48 skrev "Luc Maisonobe" <Lu...@free.fr>:
> Hi all,
>
> I have squashed a number of findbugs and checkstyle warnings introduced
> by recent changes (more than one hundred).
>
> There are a few remaining bugs for which I would like some ideas.
>
> Checkstyle errors:
>
> In class MannWhitneyUTestImpl, the javadoc for private method
> calculateAsymptoticPValue has a N parameter in the Javadoc which is
> wrong and two n1 and n2 parameters in the signature that are not
> documented. I don't know the exact meaning of n1 and n2, could someone
> fix this javadoc ?
>
> In class MathUtils, method round(double x, int scale, int
> roundingMethod) we catch RuntimeException to wrap it into
> MathRuntimeException. I think we should not and should simply let the
> RuntimeException go up. What do you think ?
>
> Findbugs errors:
>
> In CMAESOptimizer constructor, we directly store references to the
> inputSigma and boundaries parameters in internal fields, thus exposing
> internal representation. I think we should either clone the arrays or
> document the fact we will reference user arrays and set up a findbug
> exclude filter. What do you think ?
>
> In CMAESOptimizer DoubleIndex class, there is a compareTo method but no
> equals method (and if we add one, there will be no hashCode method),
> should we add them ?
>
> SerializablePair extends Pair which is not serializable and does not
> have an accessible void constructor. Should we add such a constructor
> (perhaps setting the two fields to null), should we have
> SerializablePair not extend Pair or are we sure this does work correctly
> and we should add a findbugs excude filter ?
>
> best regards,
> Luc
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
Re: [math] cleaning code
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
On Sun, Mar 27, 2011 at 11:21:51PM +0200, Gilles Sadowski wrote:
> [...]
> It's a pity that CM cannot even rely on other Commons code (such as "Lang",
> which contains a "Pair" class).
> If this is still not an option (!), I'd rather remove the "SerializablePair"
> class and change the code in "MathRuntimeException".
Done (MATH-551).
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] cleaning code
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
> In class MathUtils, method round(double x, int scale, int
> roundingMethod) we catch RuntimeException to wrap it into
> MathRuntimeException. I think we should not and should simply let the
> RuntimeException go up. What do you think ?
Agreed.
> SerializablePair extends Pair which is not serializable and does not
> have an accessible void constructor. Should we add such a constructor
> (perhaps setting the two fields to null), should we have
> SerializablePair not extend Pair or are we sure this does work correctly
> and we should add a findbugs excude filter ?
Indeed, it seems that this does not work as I expected. Adding a default
constructor does not seem to help either.
I do not favour creating classes when it means reinventing the wheel
especially when the functionality is not CM's core business. CM's "Pair"
class is only used in "sortInPlace" which is never used within CM: It was
intended as a utility so that users can easily pass sorted arrays to CM
methods that require them (e.g. interpolators).
The "SerializablePair" was a quick-and-dirty trick to allow serialization of
"MathRuntimeException" objects. It works for that purpose but it now looks
like we are led to enhance "SerializablePair" so that it behaves as its name
implies. However that's yet another piece of code that should not be
supported by CM.
It's a pity that CM cannot even rely on other Commons code (such as "Lang",
which contains a "Pair" class).
If this is still not an option (!), I'd rather remove the "SerializablePair"
class and change the code in "MathRuntimeException".
Regards,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org