LpMetric in R^d

[dcoeurjo]

PR Description

This PR introduces a generic LpMetric class on double (to fix Point<->RealPoint implicit casts when considering the distance between two (integer) Points).

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)

[dcoeurjo]

Should fix #1345 issues

[dcoeurjo]

@rolanddenis : for dvcm-2d and shortcut I need to investigate a bit more. E.g. dvcm needs a Separable metric and I don't see where the issue is

[rolanddenis]

So, the only difference with InexactPredicateLpSeparableMetric is the Point type?

[dcoeurjo]

So, the only difference with InexactPredicateLpSeparableMetric is the Point type?

And the fact that InexactPredicateLpSeparableMetric is a model of CSeparableMetric

[rolanddenis]

OK for the CMetricSpace, so the idea is to provide a base model without advanced features?

BTW, the fix of the testCube part of testTensorVoting.cpp is missing

[dcoeurjo]

OK for the CMetricSpace, so the idea is to provide a base model without advanced features?

Yes.. There is a bit of redundancies (Inexact... could inherit from LpMetric) but the Point type is different. Not great but anyway..

BTW, the fix of the testCube part of testTensorVoting.cpp is missing

^^

[rolanddenis]

@rolanddenis : for dvcm-2d and shortcut I need to investigate a bit more. E.g. dvcm needs a Separable metric and I don't see where the issue is

About that: so making LpMetric a CSeparableMetric model and using it in dvcm-2d-curvature should solve the issue ?

Moreover, looking at how LocalEstimatorFromSurfelFunctorAdapter is implemented (see https://github.com/DGtal-team/DGtal/blob/master/src/DGtal/geometry/surfaces/estimation/LocalEstimatorFromSurfelFunctorAdapter.ih#L170):

const MetricToPoint metricToPoint = std::bind( *myMetric, myEmbedder( *it ), std::placeholders::_1 );

with

typedef std::function< typename Metric::Value ( typename Metric::Point ) > MetricToPoint;

and since CCellEmbedder concept requires the returned value to be a Space::RealPoint, it implies that LocalEstimatorFromSurfelFunctorAdapter can only work with LpMetric.

Is that the wanted behaviour? (it is a real question, I'm not aware of all the usages of this class)

[dcoeurjo]

@rolanddenis : for dvcm-2d and shortcut I need to investigate a bit more. E.g. dvcm needs a Separable metric and I don't see where the issue is

About that: so making LpMetric a CSeparableMetric model and using it in dvcm-2d-curvature should solve the issue ?

dvcm-2d needs an "exact" VoronoiMap. Can you spot me where the conversion issue is in the dvcm code ?

I'd keep the following principles (maybe it should be clearer un the doc):

• LpMetric: on R^n with double (model of CMetricSpace only)
• Inexact...Separable...: on Z^n with double
• Exact...Separable...: on Z^n with exact computations

Promoting LpMetric to CSeparableMetric would probably be confusing.

Moreover, looking at how LocalEstimatorFromSurfelFunctorAdapter is implemented (see https://github.com/DGtal-team/DGtal/blob/master/src/DGtal/geometry/surfaces/estimation/LocalEstimatorFromSurfelFunctorAdapter.ih#L170):

const MetricToPoint metricToPoint = std::bind( *myMetric, myEmbedder( *it ), std::placeholders::_1 );

with

typedef std::function< typename Metric::Value ( typename Metric::Point ) > MetricToPoint;

and since CCellEmbedder concept requires the returned value to be a Space::RealPoint, it implies that LocalEstimatorFromSurfelFunctorAdapter can only work with LpMetric.

Is that the wanted behaviour? (it is a real question, I'm not aware of all the usages of this class)

I agree.. I was also thinking to force the metric in LocalEstimator.. to be a LpMetric but this would have been less generic (the only requirement is that the metric should be in R^n, and so far we have a unique model of that that is LpMetic)

[rolanddenis]

dvcm-2d needs an "exact" VoronoiMap. Can you spot me where the conversion issue is in the dvcm code ?

It comes from dvcm-2d-curvature.cpp:114 that calls the VCMDigitalSurfaceLocalEstimator::setParams method:

typedef VCMDigitalSurfaceLocalEstimator<DigitalSurfaceContainer,Metric,
    KernelFunction, CurvatureFunctor> VCMCurvatureEstimator;
...
VCMCurvatureEstimator estimator( ptrSurface );
estimator.setParams( Pointels, R, r, chi, 3.0, l2, true );

which constructs a VoronoiCovarianceMeasureOnDigitalSurface (alias VCMOnSurface) from VCMDigitalSurfaceLocalEstimator::setParams:148:

{
  mySurfelEmbedding = surfelEmbedding;
  myVCMOnSurface = CountedConstPtrOrConstPtr<VCMOnSurface>
    ( new VCMOnSurface( mySurface, mySurfelEmbedding,
                        R, r, chi_r, t, aMetric, verbose ), true );
  myGeomFct.attach( myVCMOnSurface );
}

whose constructor uses a LocalEstimatorFromSurfelFunctorAdapter here:

typedef LocalEstimatorFromSurfelFunctorAdapter< DigitalSurfaceContainer, Metric, SurfelFunctor, Functor>
    NormalEstimator;
...
NormalEstimator estimator;
...
      // get rough estimation of normal
      normals.trivialNormal = estimator.eval( it );

[rolanddenis]

I agree.. I was also thinking to force the metric in LocalEstimator.. to be a LpMetric but this would have been less generic (the only requirement is that the metric should be in R^n, and so far we have a unique model of that that is LpMetic)

Ok, so if it is necessary, we maybe should check it in LocalEstimatorFromSurfelFunctorAdapter by comparing the Value typedef of the given metric with the RealPoint of the associated space (except if it is too strict) and specify it in the documentation...

[dcoeurjo]

👌 thx.
Dvcm shouldn’t use the same metric for both the Voronoi and the neighborhood exploration. Let me update that.

[dcoeurjo]

I've pushed an edit in VCM (ping @JacquesOlivierLachaud ) to force the LpMetric in R^n to be used when performing the breath first propagation on surfaces.

[dcoeurjo]

The question is : Should be remove the Metric template parameter in LocalEstimatorFromSurfelFunctorAdapter to force the class to use the l_2 on R^n?
(and add the "p" value in the constructor) 

[dcoeurjo]

@rolanddenis ok for you ?

thx

[dcoeurjo]

thanks :)

[dcoeurjo]

Let's merge this one

[rolanddenis]

Yep! Only a question about the metric passed to LocalEstimatorFromSurfelFunctorAdapter in the VoronoiCovarianceMeasureOnDigitalSurface constructor: it can only be a L2 metric?

[dcoeurjo]

there are 2 metrics: one for the Voronoi map (must be separable) and one for the breadth first propagation (LocalEst...) qui is fixed to l2 in my edit

[dcoeurjo]

We can imagine to change the Voronoi Map metric to something else for the VCM but it won't make sense to change the propagation IMHO..

[rolanddenis]

Ok, my question was about the choice for the propagation, so all right 👍

[rolanddenis]

Didn't check the output of dvcm-2d-curvature before: I get a uniformly blue digitalized circle ... shouldn't we get a kind of color gradient depending on the curvature?

[JacquesOlivierLachaud]

You should get something like this on an ellipse (bottom of page)
https://dgtal.org/doc/stable/moduleVCM.html
On a circle, it is correct to get a uniform blue (constant curvature), but unlikely :)

[rolanddenis]

Huh yes, you're right! I found the result too simple and think there were a bug... The output seems correct for an ellipse (but not as contrasted as in the module doc but it should be due to the colormap)