Skip to content

Commit

Permalink
Removed dip::Feature::Base::Scale() again.
Browse files Browse the repository at this point in the history
This commit undoes some changes introduced in 8c58664.
I don't like making backwards-incompatible changes, but this particular
change hadn't made it to a release yet, so this shouldn't affect people
that cautiously stick to released versions.

dip::Feature::Base::Scale() was an elegant solution, but it introduced some
overhead for calling a virtual function for each feature and each object.
I've solved the issue with a few composite features that combine lengths and
areas (or surface areas and volumes) getting values in incompatible units by
detecting the situation (anisotropic pixels), and explicitly scaling the
area/volume to make the units compatible.

I think this new solution is even neater.
  • Loading branch information
crisluengo committed May 20, 2022
1 parent bb8481d commit 2dedffd
Show file tree
Hide file tree
Showing 37 changed files with 206 additions and 363 deletions.
19 changes: 10 additions & 9 deletions changelogs/diplib_next.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ title: "Changes DIPlib 3.x.x"
- `dip::Label` has a new string argument, `mode`, which can be set to `"largest"` to return a labeled
image that only contains the largest connected component in the input image.

- Added `dip::PixelSize::UnitLength()`, `dip::PixelSize::UnitSize()` and `dip::PixelSize::ForcePhysical()`,
to simplify finding the right units for measurement results.
- Added `dip::PixelSize::UnitLength()`, `dip::PixelSize::UnitSize()`, `dip::PixelSize::ForcePhysical()`,
and `dip::PixelSize::SameUnits()` to simplify finding the right units for measurement results.

- Added operators to multiply and divide two `dip::Vertex` objects. They apply an element-wise product.

Expand All @@ -42,12 +42,6 @@ title: "Changes DIPlib 3.x.x"
- The deterministic initialization for `dip::GaussianMixtureModel()` is more robust, making the
initial Gaussians overlap instead of setting their sigma to 1.

- The `dip::Feature::Base` class, the base class for all measurement features, has a new virtual
function `Scale()`, which is called for all features after they have been computed, and which
is meant to scale the measurement results with the pixel sizes. Most features no longer scale
the measurement results while computing them, as this simplifies the logic for many of the
composed features.

- `dip::ConvexHull` no longer holds a `dip::Polygon` as a private member, instead it uses `dip::Polygon`
as a base class. This makes all of the `dip::Polygon` functionality directly available on the convex
hull. `dip::ConvexHull::Polygon()` now just returns a base class reference.
Expand All @@ -60,6 +54,10 @@ title: "Changes DIPlib 3.x.x"
and takes the smoothing of the histogram into account, such that the amount of smoothing should have
little influence in the computed threshold.

- The "Mu" and "GreyMu" features no longer use the pixel sizes if the units for different image
dimensions differ. No uses of the tensor make sense if the units for the various dimensions
don't match, in this case it is more useful to report the tensor in pixel units.

### Bug fixes

- `dip::DrawPolygon2D()`, when drawing filled polygons, would skip the bottom row in the polygon. The
Expand All @@ -80,7 +78,7 @@ title: "Changes DIPlib 3.x.x"
- The "SolidArea" feature didn't take the pixel size into account. This also caused the "Roundness" feature
to report wrong values. The "SurfaceArea" feature didn't properly scale the result by the pixel size.
Several composed features (computed from other features) didn't produce correct values for anisotropic
pixels ("P2A", "Roundness", "PodczeckShapes", etc.)
pixels ("P2A", "Roundness", and "PodczeckShapes").

- `dip::div_round()` was incorrect, which caused `dip::DirectedPathOpening()` to not use certain
directions.
Expand Down Expand Up @@ -142,6 +140,9 @@ title: "Changes DIPlib 3.x.x"

- Added `dip.MeasurementTool.Configure` to allow measurement features to be configured.

- Added `AspectRatio()`, `SameUnits()`, `UnitLength()`, `UnitSize()`, `ForcePhysical()`,
and `ApproximatelyEquals()` as methods to `dip.PixelSize`.

### Changed functionality

- Operators overloaded for `dip.Image` objects can use lists of numbers as a second argument, which
Expand Down
26 changes: 16 additions & 10 deletions doc/src/features.md
Original file line number Diff line number Diff line change
Expand Up @@ -341,22 +341,25 @@ tensors are stored in an image (see \ref dip::Tensor::Shape).

For more information, see \ref dip::MomentAccumulator::SecondOrder.

These values are reported in physical units, anisotropic pixels are taken into account.
These values are reported in physical units, anisotropic pixels are taken into account, but they must
have the same units, else pixel sizes are ignored. Note that we don't handle different units along
different axes here because none of the uses of the tensor would make sense.

\subsection binary_moments_Inertia Inertia
Moments of inertia of the binary object, the eigenvalues of the tensor computed by
feature \ref binary_moments_Mu. There is one value per image dimension. The eigenvectors are sorted
largest to smallest.

These values are reported in physical units, anisotropic pixels are taken into account, but they must
have the same units.
have the same units, else pixel sizes are ignored.

\subsection binary_moments_MajorAxes MajorAxes
Principal axes of the binary object, the eigenvectors of the tensor computed by
feature \ref binary_moments_Mu. For an image with $n$ dimensions, there are $n^2$ values.
The first $n$ values are the eigenvector associated to the largest eigenvalue, etc.

This feature ignores pixel sizes, isotropic pixels are assumed.
These values are reported in physical units, anisotropic pixels are taken into account, but they must
have the same units, else pixel sizes are ignored.

\subsection binary_moments_DimensionsCube DimensionsCube
Lengths of the sides of a rectangle (2D) or box (3D) with the same moments of inertia
Expand All @@ -365,7 +368,7 @@ as the binary object. Derived from feature \ref binary_moments_Inertia.
Currently defined only for 2D and 3D images.

These values are reported in physical units, anisotropic pixels are taken into account, but they must
have the same units.
have the same units, else pixel sizes are ignored.

\subsection binary_moments_DimensionsEllipsoid DimensionsEllipsoid
Diameters of an ellipse (2D) or ellipsoid (3D) with the same moments of inertia as the
Expand All @@ -374,7 +377,7 @@ binary object. Derived from feature \ref binary_moments_Inertia.
Currently defined only for 2D and 3D images.

These values are reported in physical units, anisotropic pixels are taken into account, but they must
have the same units.
have the same units, else pixel sizes are ignored.


\comment --------------------------------------------------------------
Expand Down Expand Up @@ -409,7 +412,9 @@ For more information, see \ref dip::MomentAccumulator::SecondOrder.
Identical to feature \ref binary_moments_Mu but using the grey-value image as weighting.
`grey` must be scalar.

These values are reported in physical units, anisotropic pixels are taken into account.
These values are reported in physical units, anisotropic pixels are taken into account, but they must
have the same units, else pixel sizes are ignored. Note that we don't handle different units along
different axes here because none of the uses of the tensor would make sense.

\subsection grey_moments_GreyInertia GreyInertia
Moments of inertia of the grey-weighted object, the eigenvalues of the tensor computed by
Expand All @@ -420,7 +425,7 @@ Identical to feature \ref binary_moments_Inertia but using the grey-value image
`grey` must be scalar.

These values are reported in physical units, anisotropic pixels are taken into account, but they must
have the same units.
have the same units, else pixel sizes are ignored.

\subsection grey_moments_GreyMajorAxes GreyMajorAxes
Principal axes of the grey-weighted object, the eigenvectors of the tensor computed by
Expand All @@ -430,7 +435,8 @@ The first $n$ values are the eigenvector associated to the largest eigenvalue, e
Identical to feature \ref binary_moments_MajorAxes but using the grey-value image as weighting.
`grey` must be scalar.

This feature ignores pixel sizes, isotropic pixels are assumed.
These values are reported in physical units, anisotropic pixels are taken into account, but they must
have the same units, else pixel sizes are ignored.

\subsection grey_moments_GreyDimensionsCube GreyDimensionsCube
Lengths of the sides of a rectangle (2D) or box (3D) with the same moments of inertia
Expand All @@ -442,7 +448,7 @@ Identical to feature \ref binary_moments_DimensionsCube but using the grey-value
`grey` must be scalar.

These values are reported in physical units, anisotropic pixels are taken into account, but they must
have the same units.
have the same units, else pixel sizes are ignored.

\subsection grey_moments_GreyDimensionsEllipsoid GreyDimensionsEllipsoid
Diameters of an ellipse (2D) or ellipsoid (3D) with the same moments of inertia as the
Expand All @@ -454,4 +460,4 @@ Identical to feature \ref binary_moments_DimensionsEllipsoid but using the grey-
`grey` must be scalar.

These values are reported in physical units, anisotropic pixels are taken into account, but they must
have the same units.
have the same units, else pixel sizes are ignored.
18 changes: 18 additions & 0 deletions include/diplib/library/physical_dimensions.h
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,24 @@ class DIP_NO_EXPORT PixelSize {
return IsDefined();
}

/// Tests to see if the units are the same (and physical) for all dimensions. Note that the prefix
/// (thousands scaling) is considered part of the units.
bool SameUnits() const {
if( size_.empty() ) {
return false;
}
auto units = size_[ 0 ].units;
if( !units.IsPhysical() ) {
return false;
}
for( dip::uint ii = 1; ii < size_.size(); ++ii ) {
if( size_[ ii ].units != units ) {
return false;
}
}
return true;
}

/// Multiplies together the sizes for the first `d` dimensions.
PhysicalQuantity Product( dip::uint d ) const {
if( d == 0 ) {
Expand Down
26 changes: 9 additions & 17 deletions include/diplib/measurement.h
Original file line number Diff line number Diff line change
Expand Up @@ -725,31 +725,24 @@ class DIP_CLASS_EXPORT Base {
///
/// Note that this function can store information about the images in private data members of the
/// class, so that it is available when performing measurements. For example, it can store the
/// pixel size to be used later in \ref Scale.
/// pixel size.
///
/// !!! attention
/// This function is not expected to perform any major amount of work.
virtual ValueInformationArray Initialize( Image const& label, Image const& grey, dip::uint nObjects ) = 0;

/// \brief Called once for each object, to scale the pixel measurements according to the pixel sizes.
///
/// Typically measurements are first computed in pixels, and then scaled to the proper units. The scaling
/// is separated out, and called after all \ref Composite measurements have been computed. The `Composite`
/// measures therefore always receive input measurements in pixels.
///
/// The required scaling should be computed in the \ref Initialize call, which also computes the output
/// units. This function should only apply the scaling.
///
/// By default this function does nothing, which is suitable for some measurements.
virtual void Scale( Measurement::ValueIterator output ) {
( void ) output;
}

/// \brief All measurement features define a `Cleanup` method that is called after finishing the measurement
/// process for one image.
virtual void Cleanup() {}

// Ensure the destructor is virtual
virtual ~Base() = default;

// Silence IDE warning about there being a destructor but not these other standard functions:
Base(const Base& other) = delete;
Base(Base&& other) = delete;
Base& operator=(const Base& other) = delete;
Base& operator=(Base&& other) = delete;
};

/// \brief The pure virtual base class for all line-based measurement features.
Expand Down Expand Up @@ -837,8 +830,7 @@ class DIP_CLASS_EXPORT Composite : public Base {
virtual StringArray Dependencies() = 0;

/// \brief Called once for each object, the input `dependencies` object contains the measurements
/// for the object from all the features in the \ref Dependencies() list. The measurements are always
/// un pixel units, not yet scaled by the pixel sizes.
/// for the object from all the features in the \ref Dependencies() list.
virtual void Compose( Measurement::IteratorObject& dependencies, Measurement::ValueIterator output ) = 0;
};

Expand Down
12 changes: 9 additions & 3 deletions pydip/src/pydip.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,16 @@ PYBIND11_MODULE( PyDIP_bin, m ) {
pixSz.def( "Invert", py::overload_cast< dip::uint >( &dip::PixelSize::Invert ), "d"_a );
pixSz.def( "Invert", py::overload_cast<>( &dip::PixelSize::Invert ));
pixSz.def( "IsIsotropic", &dip::PixelSize::IsIsotropic );
pixSz.def( "AspectRatio", &dip::PixelSize::AspectRatio, "d"_a );
pixSz.def( "IsDefined", &dip::PixelSize::IsDefined );
pixSz.def( "Product", &dip::PixelSize::Product );
pixSz.def( "ToPixels", &dip::PixelSize::ToPixels );
pixSz.def( "ToPhysical", &dip::PixelSize::ToPhysical );
pixSz.def( "SameUnits", &dip::PixelSize::SameUnits );
pixSz.def( "Product", &dip::PixelSize::Product, "d"_a );
pixSz.def( "UnitLength", &dip::PixelSize::UnitLength );
pixSz.def( "UnitSize", &dip::PixelSize::UnitSize, "d"_a );
pixSz.def( "ForcePhysical", &dip::PixelSize::ForcePhysical );
pixSz.def( "ApproximatelyEquals", &dip::PixelSize::ApproximatelyEquals, "rhs"_a, "nDims"_a, "tolerance"_a = 1e-6 );
pixSz.def( "ToPixels", &dip::PixelSize::ToPixels, "in"_a );
pixSz.def( "ToPhysical", &dip::PixelSize::ToPhysical, "in"_a );
py::implicitly_convertible< dip::PhysicalQuantity, dip::PixelSize >();
py::implicitly_convertible< dip::PhysicalQuantityArray, dip::PixelSize >();

Expand Down
6 changes: 1 addition & 5 deletions src/measurement/feature_bending_energy.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,7 @@ class FeatureBendingEnergy : public ChainCodeBased {
}

virtual void Measure( ChainCode const& chainCode, Measurement::ValueIterator output ) override {
*output = chainCode.BendingEnergy();
}

virtual void Scale( Measurement::ValueIterator output ) override {
*output *= scale_;
*output = chainCode.BendingEnergy() * scale_;
}

private:
Expand Down
8 changes: 1 addition & 7 deletions src/measurement/feature_cartesian_box.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,17 +82,11 @@ class FeatureCartesianBox : public LineBased {
}
} else {
for( dip::uint ii = 0; ii < nD_; ++ii ) {
output[ ii ] = static_cast< dfloat >( data[ ii ].max - data[ ii ].min + 1 );
output[ ii ] = static_cast< dfloat >( data[ ii ].max - data[ ii ].min + 1 ) * scales_[ ii ];
}
}
}

virtual void Scale( Measurement::ValueIterator output ) override {
for( dip::uint ii = 0; ii < nD_; ++ii ) {
output[ ii ] *= scales_[ ii ];
}
}

virtual void Cleanup() override {
data_.clear();
data_.shrink_to_fit();
Expand Down
8 changes: 1 addition & 7 deletions src/measurement/feature_center.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,11 @@ class FeatureCenter : public LineBased {
}
} else {
for( dip::uint ii = 0; ii < nD_; ++ii ) {
output[ ii ] = data[ ii ] / data[ nD_ ];
output[ ii ] = data[ ii ] / data[ nD_ ] * scales_[ ii ];
}
}
}

virtual void Scale( Measurement::ValueIterator output ) override {
for( dip::uint ii = 0; ii < nD_; ++ii ) {
output[ ii ] *= scales_[ ii ];
}
}

virtual void Cleanup() override {
data_.clear();
data_.shrink_to_fit();
Expand Down
Loading

0 comments on commit 2dedffd

Please sign in to comment.