Skip to content

Commit

Permalink
Fixed addition for dip::StatisticsAccumulator and dip::VarianceAccumu…
Browse files Browse the repository at this point in the history
…lator.
  • Loading branch information
crisluengo committed Jul 3, 2024
1 parent 63accf4 commit 5ef3954
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 22 deletions.
4 changes: 4 additions & 0 deletions changelogs/diplib_next.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ date: 2020-00-00
- `dip::GetOptimalDFTSize()` could loop indefinitely for inputs close to the maximum `dip::uint` value, because
of an integer overflow in arithmetic.

- The classes `dip::StatisticsAccumulator` and `dip::VarianceAccumulator` divided by 0 when adding two empty
accumulators together, producing NaN values as output. This caused the function `dip::SampleStatistics()`
to sometimes produce NaN statistics when called with a mask image.

### Updated dependencies

### Build changes
Expand Down
44 changes: 32 additions & 12 deletions include/diplib/accumulators.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ class DIP_NO_EXPORT StatisticsAccumulator {

/// Combine two accumulators
StatisticsAccumulator& operator+=( StatisticsAccumulator const& b ) {
if( b.n_ == 0 ) {
return *this;
}
if( n_ == 0 ) {
*this = b;
return *this;
}
// The code below assumes n_ + b.n_ > 0
dfloat an = static_cast< dfloat >( n_ );
dfloat an2 = an * an;
dfloat bn = static_cast< dfloat >( b.n_ );
Expand Down Expand Up @@ -208,6 +216,14 @@ class DIP_NO_EXPORT VarianceAccumulator {

/// Combine two accumulators
VarianceAccumulator& operator+=( VarianceAccumulator const& b ) {
if( b.n_ == 0 ) {
return *this;
}
if( n_ == 0 ) {
*this = b;
return *this;
}
// The code below assumes n_ + b.n_ > 0
dfloat oldn = static_cast< dfloat >( n_ );
n_ += b.n_;
dfloat n = static_cast< dfloat >( n_ );
Expand Down Expand Up @@ -384,21 +400,25 @@ class DIP_NO_EXPORT CovarianceAccumulator {

/// Combine two accumulators
CovarianceAccumulator& operator+=( const CovarianceAccumulator& other ) {
if( other.n_ == 0 ) {
return *this;
}
if( n_ == 0 ) {
*this = other; // copy over the data
} else if( other.n_ > 0 ) {
dip::uint intN = n_ + other.n_;
dfloat N = static_cast< dfloat >( intN );
dfloat dx = other.meanx_ - meanx_;
dfloat dy = other.meany_ - meany_;
meanx_ = ( static_cast< dfloat >( n_ ) * meanx_ + static_cast< dfloat >( other.n_ ) * other.meanx_ ) / N;
meany_ = ( static_cast< dfloat >( n_ ) * meany_ + static_cast< dfloat >( other.n_ ) * other.meany_ ) / N;
dfloat fN = static_cast< dfloat >( n_ * other.n_ ) / N;
m2x_ += other.m2x_ + dx * dx * fN;
m2y_ += other.m2y_ + dy * dy * fN;
C_ += other.C_ + dx * dy * fN;
n_ = intN;
return *this;
}
// The code below assumes n_ + b.n_ > 0
dip::uint intN = n_ + other.n_;
dfloat N = static_cast< dfloat >( intN );
dfloat dx = other.meanx_ - meanx_;
dfloat dy = other.meany_ - meany_;
meanx_ = ( static_cast< dfloat >( n_ ) * meanx_ + static_cast< dfloat >( other.n_ ) * other.meanx_ ) / N;
meany_ = ( static_cast< dfloat >( n_ ) * meany_ + static_cast< dfloat >( other.n_ ) * other.meany_ ) / N;
dfloat fN = static_cast< dfloat >( n_ * other.n_ ) / N;
m2x_ += other.m2x_ + dx * dx * fN;
m2y_ += other.m2y_ + dy * dy * fN;
C_ += other.C_ + dx * dy * fN;
n_ = intN;
return *this;
}

Expand Down
15 changes: 5 additions & 10 deletions src/statistics/statistics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ class MaximumAndMinimumLineFilter : public MaximumAndMinimumLineFilterBase {
dip::uint GetNumberOfOperations( dip::uint /**/, dip::uint /**/, dip::uint /**/ ) override { return 3; }
void Filter( Framework::ScanLineFilterParameters const& params ) override {
TPI const* in = static_cast< TPI const* >( params.inBuffer[ 0 ].buffer );
MinMaxAccumulator vars;
MinMaxAccumulator& vars = accArray_[ params.thread ];
auto bufferLength = params.bufferLength;
auto inStride = params.inBuffer[ 0 ].stride;
if( params.inBuffer.size() > 1 ) {
Expand All @@ -401,7 +401,6 @@ class MaximumAndMinimumLineFilter : public MaximumAndMinimumLineFilterBase {
vars.Push( static_cast< dfloat >( *in ));
}
}
accArray_[ params.thread ] += vars;
}
void SetNumberOfThreads( dip::uint threads ) override {
accArray_.resize( threads );
Expand Down Expand Up @@ -507,7 +506,7 @@ class SampleStatisticsLineFilter : public SampleStatisticsLineFilterBase {
dip::uint GetNumberOfOperations( dip::uint /**/, dip::uint /**/, dip::uint /**/ ) override { return 23; }
void Filter( Framework::ScanLineFilterParameters const& params ) override {
TPI const* in = static_cast< TPI const* >( params.inBuffer[ 0 ].buffer );
StatisticsAccumulator vars;
StatisticsAccumulator& vars = accArray_[ params.thread ];
auto bufferLength = params.bufferLength;
auto inStride = params.inBuffer[ 0 ].stride;
if( params.inBuffer.size() > 1 ) {
Expand All @@ -528,7 +527,6 @@ class SampleStatisticsLineFilter : public SampleStatisticsLineFilterBase {
in += inStride;
}
}
accArray_[ params.thread ] += vars;
}
void SetNumberOfThreads( dip::uint threads ) override {
accArray_.resize( threads );
Expand Down Expand Up @@ -572,7 +570,7 @@ class CovarianceLineFilter : public CovarianceLineFilterBase {
void Filter( Framework::ScanLineFilterParameters const& params ) override {
TPI const* in1 = static_cast< TPI const* >( params.inBuffer[ 0 ].buffer );
TPI const* in2 = static_cast< TPI const* >( params.inBuffer[ 1 ].buffer );
CovarianceAccumulator vars;
CovarianceAccumulator& vars = accArray_[ params.thread ];
auto bufferLength = params.bufferLength;
auto in1Stride = params.inBuffer[ 0 ].stride;
auto in2Stride = params.inBuffer[ 1 ].stride;
Expand All @@ -596,7 +594,6 @@ class CovarianceLineFilter : public CovarianceLineFilterBase {
in2 += in2Stride;
}
}
accArray_[ params.thread ] += vars;
}
void SetNumberOfThreads( dip::uint threads ) override {
accArray_.resize( threads );
Expand Down Expand Up @@ -731,7 +728,7 @@ class CenterOfMassLineFilter : public CenterOfMassLineFilterBase {
dip::uint GetNumberOfOperations( dip::uint /**/, dip::uint /**/, dip::uint /**/ ) override { return nD_ + 1; }
void Filter( Framework::ScanLineFilterParameters const& params ) override {
TPI const* in = static_cast< TPI const* >( params.inBuffer[ 0 ].buffer );
FloatArray vars( nD_ + 1, 0.0 );
FloatArray& vars = accArray_[ params.thread ];
auto bufferLength = params.bufferLength;
auto inStride = params.inBuffer[ 0 ].stride;
UnsignedArray pos = params.position;
Expand Down Expand Up @@ -762,7 +759,6 @@ class CenterOfMassLineFilter : public CenterOfMassLineFilterBase {
++( pos[ procDim ] );
}
}
accArray_[ params.thread ] += vars;
}
CenterOfMassLineFilter( dip::uint nD ) : nD_( nD ) {}
void SetNumberOfThreads( dip::uint threads ) override {
Expand Down Expand Up @@ -820,7 +816,7 @@ class MomentsLineFilter : public MomentsLineFilterBase {
}
void Filter( Framework::ScanLineFilterParameters const& params ) override {
TPI const* in = static_cast< TPI const* >( params.inBuffer[ 0 ].buffer );
MomentAccumulator vars( nD_ );
MomentAccumulator& vars = accArray_[ params.thread ];
auto bufferLength = params.bufferLength;
auto inStride = params.inBuffer[ 0 ].stride;
FloatArray pos{ params.position };
Expand All @@ -845,7 +841,6 @@ class MomentsLineFilter : public MomentsLineFilterBase {
++( pos[ procDim ] );
}
}
accArray_[ params.thread ] += vars;
}
MomentsLineFilter( dip::uint nD ) : nD_( nD ) {}
void SetNumberOfThreads( dip::uint threads ) override {
Expand Down

0 comments on commit 5ef3954

Please sign in to comment.