Fixing PointVector implicit conversions

[rolanddenis]

PR Description

Fixing conversions between PointVector with different Euclidean Ring type and storage (see #1341).
Automatic Euclidean Ring return type for compatible operations.

Note that all external operators (+, -, <, ...) and functions (dotProduct, crossProduct, ...) that are related to PointVector are declared as friend of this class. It is not necessary since those operators and functions never access to protected/private members but it allows to have them documented on the same page as PointVector in a "Friends" section.

Checklist

• Unit-test of your feature with Catch.
• Doxygen documentation of the code completed (classes, methods, types, members...)
• Documentation module page added or updated.
• New entry in the ChangeLog.md added.
• No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
• All continuous integration tests pass (Travis & appveyor)
• Clean commit history

PointVector checklist

• Assignment operator
• Unary & binary functor conversion constructors
• Arithmetic binary operators
• Arithmetic unary operators with assignment (+=, -=,...)
• Comparison operators (using lexicographical order)
• Other binary functions on PointVector (dot, crossProduct, inf,...) . Also external versions.
• Adding comparison tests
• partialCopy and partialEqual
• New functors to ease the choice of rounding mode without specifying type (like with std::round)
• crossProduct of 2D vectors returns a scalar. This is a critical change that may lead to compilation errors (e.g. https://github.com/rolanddenis/DGtal/blob/fix_pointvector/src/DGtal/shapes/Mesh.ih#L464)

Conversion fixes

Q&A

Why is there a conversion issue ?

When a PointVector<dim,T> is converted into another PointVector<dim,U> in a implicit way (e.g. when passing the first type to a function accepting the second type) and if converting from T to U is ambiguous (e.g. from double to int), then this conversion will fail.

Which conversions are valid ?

The conversion follows the implicit arithmetic conversion rules (see https://en.cppreference.com/w/cpp/language/operator_arithmetic#Conversions), that is T can be converted to U if T + U has type U. This type deduction is explained in the link above and is quite natural except for the signed/unsigned conversion: if T and U are both integers with same precision but one is signed and the other unsigned, then T + U is unsigned. Thus, conversion from signed int to unsigned int is valid while the contrary, unsigned int to signed int, is invalid.

How to fix a conversion issue ?

It depends on what the initial code was supposed to do:

• if a simple cast was meant (beware of numerical errors !), then you can use static_cast or explicitly call the constructor: instead of

PointVector<2, int> p1;
PointVector<2, double> p2; 
...
p1 = p2; // Implicit conversion from double to int requested => compiler error !

write

PointVector<2, int> p1;
PointVector<2, double> p2; 
...
p1 = PointVector<2, int>(p2); // Explicit call to the appropriate constructor. 

• if the value must be modified before the cast, like using a rounding function, you can explicitly call the constructor with the appropriate functor as second argument (take a look at the new functors introduced in src/DGtal/base/BasicFunctors.h):
  instead of

PointVector<2, int> p1;
PointVector<2, double> p2; 
...
p1 = p2; // Implicit conversion from double to int requested => compiler error !

write

PointVector<2, int> p1;
PointVector<2, double> p2; 
...
// Explicit call to the constructor with conversion functor.
p1 = PointVector<2, int>(p2, functors::Round<>());

So basically, in most cases, we just need to know if the conversion should be fixed using static_cast, trunc, ceil, floor, round or something more exotic.

Why are some developers pinned to specific issues ?

I've just use git blame on erroneous lines 😉
I need to know if the proposed fix is valid or, if the issue is not already fixed (unchecked box), I need to know how to fix it (basically, what conversion function should be used).

Remaining issues

In DGtal

• Point2DEmbedderIn3D constructor: using std::round to convert from double to int components. testBasicPointFunctors now fails... (see Incertitude on the chosen basis in Point2DEmbedderIn3D #1348 and 2abba3d, ping @kerautret)
• DGtal::functions::Hull2D::computeHProjDistance, computeVProjDistance and computeEuclideanDistance (see 7429806, ping @kerautret).
• testAlphaThickSegmentComputer.cpp: using std::round to convert from double to int components (see b71531a, ping @kerautret).
• while compiling testAlphaThickSegmentComputer.cpp, there is a conversion error in AlphaThickSegmentComputer.ih. aLastExtrPt is of type InputPoint (template parameter of AlphaThickSegmentComputer) while pointEnvConv is of type PointD, that is always PointVector<2, double> (ping @kerautret)
• while compiling testCombinDSS.cpp, there is a conversion error in OneBalancedWordComputer.ih. The right-hand-side has Euclidean ring of type unsigned int while the left-hand-side is signed that is surprisingly not a valid conversion ... Storing myNbRepeat as an int or simply casting it to int on this line should be enough to fix this issue. Do you agree @xprov ?
• while compiling testTrueLocalEstimator.cpp and shapeGridCurveEstimator.cpp, there is a conversion error in ParametricShapeArcLengthFunctor.h. The shape bounds (a Ball2D) are of type Space::RealPoint and v is a Vector, aka Space::Point (thanks @troussil).
• while compiling testDistanceTransformation.cpp, there is a conversion error when passing the AccFlower2D shape bounds (aka Space::RealPoint) as the bounds of a HyperRectDomain (thanks @troussil).
• while compiling testTensorVoting.cpp, testLocalEstimatorFromFunctorAdapter.cpp, testSphericalHoughNormalVectorEstimator.cpp, exampleEstimatorFromSurfelFunctors.cpp, dvcm-2d-curvature.cpp and shortcuts-geometry.cpp, there is a conversion error in LocalEstimatorFromSurfelFunctorAdapter.ih. When binding the first parameter of *myMetric to myEmbedder(*it), the problem is that the first should be a PointVector<3, int> while the second is a PointVector<3, double> (Value type of a CCellEmbedder model) (thanks @dcoeurjo)
• while compiling testDistancePropagation.cpp there is a conversion error when binding the first argument of a distance (aka Space::Point) to the result of a CanonicEmbedder (aka Space::RealPoint) (thanks @dcoeurjo).
• while compiling testMesh.cpp, there is a conversion error in Mesh.ih. The problem is that the Mesh vertex have integer component (is that always the case ?) and those components are here scaled by a double thus leading to an invalid conversion (ping @kerautret).
• while compiling testMeshVoxelization.cpp and exampleMeshVoxelizer.cpp, there is a conversion error in MeshVoxelizer.ih. A Space::RealPoint is assigned to a Space::Point (thanks @hadjiszs).
• while compiling exampleAlphaThickSegment.cpp, there is a conversion error in AlphaThickSegmentComputer.ih. A PointD (aka PointVector<2, double>) is assigned to a InputPoint (aka Space::Point here but it is not clear if it always has integer components) (ping @kerautret).
• while compiling exampleConvexHull2D.cpp, there is a conversion error: the bounds of a Ball2D shape (aka Space::RealPoint) are given as parameters to a HyperRectDomain constructor (Space::Point expected) (thanks @troussil).
• while compiling exampleAlphaShape.cpp, there is a conversion error: the bounds of a Ball2D shape (aka Space::RealPoint) are given as parameters to a HyperRectDomain constructor (Space::Point expected) (thanks @troussil).
• tests/geometry/curves/testAlphaThickSegmentComputer.cpp
• tests/geometry/curves/testMonge.cpp
• examples/io/viewers/viewer3D-8bis-2Dimages.cpp
• examples/geometry/curves/exampleGridCurve3d.cpp
• examples/tutorial-examples/volDTGranulo.cpp
• examples/graph/volDistanceTraversal.cpp

In DGtalTools

• estimators/2dLocalEstimators.cpp
• estimators/volSurfaceRegularization.cpp
• converters/mesh2heightfield.cpp
• converters/img2freeman.cpp
• converters/vol2heightfield.cpp
• visualisation/3dCurvatureViewerNoise.cpp
• visualisation/3dCurvatureViewer.cpp
• visualisation/3dVolViewer.cpp

In DGtalTools-contrib

• geometry3d/basicEditMesh.cpp

[troussil]

For ParametricShapeArcLengthFunctor.h, you can use RealPoint instead of Vector.

[rolanddenis]

For ParametricShapeArcLengthFunctor.h, you can use RealPoint instead of Vector.

Ok, thanks ! By the way, what is the purpose of the int32 cast followed by a int64 cast and a double cast in the next line?

double n = (double) NumberTraits<Integer>::castToInt64_t( (const DGtal::int32_t)v.norm(Vector::L_infty) );
unsigned int nbSamples = (unsigned int) ceil( n*100 );

[troussil]

I have no idea... If you use RealPoint, then
double n = v.norm(RealPoint::L_infty)
is enough.

[rolanddenis]

Ok, thanks 😉

[troussil]

To solve issues involved in exampleConvexHull2D.cpp and exampleAlphaShape.cpp (and maybe other similar issues), I suggest to add a constructor to HyperRectDomain that takes input bounds of type Space::RealPoint. Something like (using C++11 syntax, but I didn't test the code):

DGtal::HyperRectDomain< TSpace >::HyperRectDomain (
  const typename TSpace::RealPoint& aPointA,
  const typename TSpace::RealPoint& aPointB ) : 
    DGtal::HyperRectDomain< TSpace >::HyperRectDomain (
      Point(aPointA, functors::Floor<>()), 
      Point(aPointB, functors::Ceil<>()) )
{}

[rolanddenis]

Ok, good idea ! However, it is not clear for me if, for example, [3.4, 3.6] should lead to an empty 1D domain (since no integer point is within the given interval) or to the [3, 4] domain ?

[troussil]

In my view, it is better to have a digital bounding box larger than the real one (for digitization purpose for instance). That's why I suggest to do "floor" on the lower bound and "ceil" on the upper one.

So [3.4, 3.6] -> [3,4] is fine for me.

[rolanddenis]

Ok, thanks !

[dcoeurjo]

hi @rolanddenis and @kerautret, can we push forward a bit this PR ?

what remains to be done ?

[kerautret]

hi David yes for me all fine, just merge the branch of src remote of @rolanddenis
For the tools it just contain some prog to fix I will make point

[rolanddenis]

Sorry, I was on vacation for few days, I will reviews your PRs later today or tomorrow !

[rolanddenis]

I've added the "Ready to review" tag to this PR since the proposed modifications to the PointVector class are in a final state.

Since the last accepted PR, the main Travis jobs (that includes only the tests) run now successfully, thanks everyone 🎉 🎊

It remains testMonge (not tested since it requires CGAL) and testBasicPointFunctors (temporary but invalid fix) to be fixed, and also several examples files and tools.

We are almost at the end of this PR!

[dcoeurjo]

What is the problem with testMonge ? The numerical values ?

[dcoeurjo]

you haven't use my testMonge edit ?

[rolanddenis]

Indeed, just merged your PR, thanks!

[rolanddenis]

About the issues in DGtal, it seems that last commits solve all remaining examples 😉

It just remains testBasicPointFunctors...

[dcoeurjo]

Thx! Let’s wait for Travis and I’ll merge. Huge thank for this painful pandora box ;)

I’ve PR some edits in Bertrand’s DGtaltools fixes. Tools should be ok.

[dcoeurjo]

For the remaining failing test.. can you please open up a new issue ?

[dcoeurjo]

Tools failing but it’s normal ;)
Thx merging

[rolanddenis]

Yep, already done in #1348 😉

[rolanddenis]

Oops, it seems that I just forget to finish the documentation of PointVector (the FIXME), I will look at that next week 😅