EPFL-Center-for-Imaging / splinebox

A python package for fitting splines.
BSD 3-Clause "New" or "Revised" License
14 stars 0 forks source link

Is the centroid implemented correctly? #10

Closed faymanns closed 7 months ago

faymanns commented 7 months ago

In the original version, the centroid is implemented as an average of the coefficients.

def centroid(self):
    centroid=np.zeros((2))        
    for k in range(0, self.M):
         centroid+=self.coefs[k]
    return centroid

It is not clear to me if this is consistant with the following definition of the centroid:

$$c = \frac{\int_lxdx}{\int_ldx}$$

$$= \frac{\int_tf(t)f\'(t)dt}{\int_tf\'(t)dt}$$

Here $l$ is the line/curve and $f(t)$ is the spline describing the curve.

Should we instead average the knots or calculate the integrals of the definition?

I am also not sure what the use cases are for this function? Is it just a helper function for the scale and rotate methods?

vuhlmann commented 7 months ago

This is indeed misleading - this function is only meant to help geometric operations like scale, rotate, and translate. It does not return the centroid of the planar curve but the centroid of the control points. This is the sensible thing to do in an interactive setting where the elements that are accessible/visible/editable by the user are the knots or the control points: if, as a user, I select all control points to rotate them, I'd expect that the center of rotation is the centroid of the points I selected - not anything else.

I believe it is useful to keep this, but it should be renamed for clarity. There could also be another function (to be implemented) calculating the actual centroid of the curve - though I'm not sure how useful it would be since this can also be very quickly approximated by sampling the curve and averaging the sample points (most likely always much quicker than solving an integral).

faymanns commented 7 months ago

I see! I'll rename it to coeff_centroid. Perhaps we can even make it a private method and expose the functionality by adding a keyword argument called centered=True to the rotate method.

vuhlmann commented 7 months ago

This is a good suggestion!

faymanns commented 7 months ago

Implemented in 9befadb and b876c9e.