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 2008/01/12 21:59:44 UTC
[math] understanding BigMatrixImpl.TOO_SMALL field
I am performing a new pass on removing findbugs warnings.
One warning occurs in both BigMatrixImpl and RealMatrixImpl, it is about
the protected field TOO_SMALL that according to findbugs should be
final. I agree with it. This field is also protected whereas I would
prefer it to be private. It is used in only once in each class.
I don't see the point for a protected non final field. Is it intended to
be changed by derived classes ? Then shouln't there be a method for that
? Would you mind if I change the field to private final ?
Luc
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] understanding BigMatrixImpl.TOO_SMALL field
Posted by Phil Steitz <ph...@gmail.com>.
On Jan 13, 2008 11:30 AM, sebb <se...@gmail.com> wrote:
> On 12/01/2008, Phil Steitz <ph...@gmail.com> wrote:
> > On Jan 12, 2008 3:45 PM, Rahul Akolkar <ra...@gmail.com> wrote:
> > > On 1/12/08, Luc Maisonobe <Lu...@free.fr> wrote:
> > > > One comment about this change: this would break compatibility with
> > > > version 1.1 and the clirr plugin flags this as an error.
> > > <snip/>
> > >
>
> Seems to me that clirr compatibility should not be at the expense of
> bug-free code.
>
> > > That was my initial reaction as well when I read your first note
> > > (below). In general, we try to avoid incompatible changes (there are
> > > specific cases where we can attempt to push for exceptions, such as
> > > blatant bugs and/or v0.x releases).
> > >
> > > About the clirr errors you mention in the following paragraph, I
> > > suspect they will come up for discussion at the next release unless
> > > they are addressed or its a major release etc.
> > >
> > Yes. We need to fix these things somehow. The decision to make these
> > fields protected in 1.0 was unfortunate, but we need to find a way to
> > maintain compatibility while we deprecate things. Sorry, I think this
> > was my mistake. I will look into how to fix the problems in the stats
> > package. IIRC, the BigMatrix.TOO_SMALL issue came up when we were
> > cutting 1.1 and we concluded then that there was no way to fix it
> > without breaking compatibility, which is why it is still there. Might
> > be best to deprecate, so we can then make it both private and final in
> > 2.0.
> >
>
> Making the field final will prevent any bugs associated with changes
> to the value.
>
> This will obviously cause a clirr error, but it seems to me that any
> code that relies on being able to change the field is already in
> error, and therefore the clirr error should be overridden in this
> case.
>
I agree with the sentiment here, but don't like the idea of
incompatible change in a point release, so would prefer deprecate as
part of public API and then fix.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] understanding BigMatrixImpl.TOO_SMALL field
Posted by sebb <se...@gmail.com>.
On 12/01/2008, Phil Steitz <ph...@gmail.com> wrote:
> On Jan 12, 2008 3:45 PM, Rahul Akolkar <ra...@gmail.com> wrote:
> > On 1/12/08, Luc Maisonobe <Lu...@free.fr> wrote:
> > > One comment about this change: this would break compatibility with
> > > version 1.1 and the clirr plugin flags this as an error.
> > <snip/>
> >
Seems to me that clirr compatibility should not be at the expense of
bug-free code.
> > That was my initial reaction as well when I read your first note
> > (below). In general, we try to avoid incompatible changes (there are
> > specific cases where we can attempt to push for exceptions, such as
> > blatant bugs and/or v0.x releases).
> >
> > About the clirr errors you mention in the following paragraph, I
> > suspect they will come up for discussion at the next release unless
> > they are addressed or its a major release etc.
> >
> Yes. We need to fix these things somehow. The decision to make these
> fields protected in 1.0 was unfortunate, but we need to find a way to
> maintain compatibility while we deprecate things. Sorry, I think this
> was my mistake. I will look into how to fix the problems in the stats
> package. IIRC, the BigMatrix.TOO_SMALL issue came up when we were
> cutting 1.1 and we concluded then that there was no way to fix it
> without breaking compatibility, which is why it is still there. Might
> be best to deprecate, so we can then make it both private and final in
> 2.0.
>
Making the field final will prevent any bugs associated with changes
to the value.
This will obviously cause a clirr error, but it seems to me that any
code that relies on being able to change the field is already in
error, and therefore the clirr error should be overridden in this
case.
Changing from protected to private is a different matter, as code may
want to read the field.
> Phil
>
> ---------------------------------------------------------------------
> 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] understanding BigMatrixImpl.TOO_SMALL field
Posted by Phil Steitz <ph...@gmail.com>.
On Jan 12, 2008 4:49 PM, Phil Steitz <ph...@gmail.com> wrote:
> On Jan 12, 2008 3:45 PM, Rahul Akolkar <ra...@gmail.com> wrote:
> > On 1/12/08, Luc Maisonobe <Lu...@free.fr> wrote:
> > > One comment about this change: this would break compatibility with
> > > version 1.1 and the clirr plugin flags this as an error.
> > <snip/>
> >
> > That was my initial reaction as well when I read your first note
> > (below). In general, we try to avoid incompatible changes (there are
> > specific cases where we can attempt to push for exceptions, such as
> > blatant bugs and/or v0.x releases).
> >
> > About the clirr errors you mention in the following paragraph, I
> > suspect they will come up for discussion at the next release unless
> > they are addressed or its a major release etc.
> >
> Yes. We need to fix these things somehow. The decision to make these
> fields protected in 1.0 was unfortunate, but we need to find a way to
> maintain compatibility while we deprecate things. Sorry, I think this
> was my mistake. I will look into how to fix the problems in the stats
> package.
I remember now what is going on here. The protected fields "removed"
from DescriptiveStatisticsImpl, SummaryStatisticsImpl were moved to
their superclasses, so are still available to them and their
subclasses. I think this is OK, just should be commented in release
notes.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] understanding BigMatrixImpl.TOO_SMALL field
Posted by Phil Steitz <ph...@gmail.com>.
On Jan 12, 2008 3:45 PM, Rahul Akolkar <ra...@gmail.com> wrote:
> On 1/12/08, Luc Maisonobe <Lu...@free.fr> wrote:
> > One comment about this change: this would break compatibility with
> > version 1.1 and the clirr plugin flags this as an error.
> <snip/>
>
> That was my initial reaction as well when I read your first note
> (below). In general, we try to avoid incompatible changes (there are
> specific cases where we can attempt to push for exceptions, such as
> blatant bugs and/or v0.x releases).
>
> About the clirr errors you mention in the following paragraph, I
> suspect they will come up for discussion at the next release unless
> they are addressed or its a major release etc.
>
Yes. We need to fix these things somehow. The decision to make these
fields protected in 1.0 was unfortunate, but we need to find a way to
maintain compatibility while we deprecate things. Sorry, I think this
was my mistake. I will look into how to fix the problems in the stats
package. IIRC, the BigMatrix.TOO_SMALL issue came up when we were
cutting 1.1 and we concluded then that there was no way to fix it
without breaking compatibility, which is why it is still there. Might
be best to deprecate, so we can then make it both private and final in
2.0.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] understanding BigMatrixImpl.TOO_SMALL field
Posted by Rahul Akolkar <ra...@gmail.com>.
On 1/12/08, Luc Maisonobe <Lu...@free.fr> wrote:
> One comment about this change: this would break compatibility with
> version 1.1 and the clirr plugin flags this as an error.
<snip/>
That was my initial reaction as well when I read your first note
(below). In general, we try to avoid incompatible changes (there are
specific cases where we can attempt to push for exceptions, such as
blatant bugs and/or v0.x releases).
About the clirr errors you mention in the following paragraph, I
suspect they will come up for discussion at the next release unless
they are addressed or its a major release etc.
-Rahul
> However we
> already have clirr errors in the report since the 2 protected fields eDA
> and windowSize have been removed from the now deprecated
> DescriptiveStatisticsImpl class and since the 10 protected fields
> geoMean, max, mean, min, n, secondMoment, sum , sumLog, sumsq and
> variance from the now deprecated SummaryStatisticsImpl class.
>
> Luc
>
> Luc Maisonobe wrote:
> > I am performing a new pass on removing findbugs warnings.
> > One warning occurs in both BigMatrixImpl and RealMatrixImpl, it is about
> > the protected field TOO_SMALL that according to findbugs should be
> > final. I agree with it. This field is also protected whereas I would
> > prefer it to be private. It is used in only once in each class.
> >
> > I don't see the point for a protected non final field. Is it intended to
> > be changed by derived classes ? Then shouln't there be a method for that
> > ? Would you mind if I change the field to private final ?
> >
> > Luc
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] understanding BigMatrixImpl.TOO_SMALL field
Posted by Luc Maisonobe <Lu...@free.fr>.
One comment about this change: this would break compatibility with
version 1.1 and the clirr plugin flags this as an error. However we
already have clirr errors in the report since the 2 protected fields eDA
and windowSize have been removed from the now deprecated
DescriptiveStatisticsImpl class and since the 10 protected fields
geoMean, max, mean, min, n, secondMoment, sum , sumLog, sumsq and
variance from the now deprecated SummaryStatisticsImpl class.
Luc
Luc Maisonobe wrote:
> I am performing a new pass on removing findbugs warnings.
> One warning occurs in both BigMatrixImpl and RealMatrixImpl, it is about
> the protected field TOO_SMALL that according to findbugs should be
> final. I agree with it. This field is also protected whereas I would
> prefer it to be private. It is used in only once in each class.
>
> I don't see the point for a protected non final field. Is it intended to
> be changed by derived classes ? Then shouln't there be a method for that
> ? Would you mind if I change the field to private final ?
>
> 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