Open oleg-alexandrov opened 8 years ago
I agree with your analysis.
I would assume that the apply concept was wrongly transferred from the DataPointsFilters
, to the Transformations
and never properly tested.
However, I would fix it differently because if only chains of size 1 can be applied there is no need for chains.
So one fix would be to remove the Transformations class entirely. Another would be to require as many TransformationParameter
objects as there are transformations in the chain as arguments for compute, but it doesn't seem obvious how to do that nicely and intuitively with c++. I guess the most intuitive way here would be to change the Transformations
class such that it actually stores transformation parameters for each transformation - maybe mutable from outside. The apply method would then not get any extra parameters in addition to the mutable cloud. This way the overall behaviour would be much more in line with the DataPointsFilters
class. One could leave a function taking an additional TransformationParameter object and checking the chain length to be one, as you suggested, for backward compatibility over some transition phase.
Which way to go for the library is @pomerlef to decide. Currently this might take some days AFAIK.
I do agree that the API itself needs modification. Chains here make no sense.
Alright, I'm trying to slowly catching up with the maintenance...
Thanks @oleg-alexandrov to point that out. The idea was to offer the possibility to the user to build a list of transformations and to apply all of them with one call. Not much thoughts were put into that as we dealt with only one Transformations
class for many years. Now that I'm thinking about it, it was most probably an over optimisation as I'm not sure if it make sense at all. There is a tight connection between the Transformations
class used and the ErrorMinimizer
used. Since only one ErrorMinimizer
is allowed, it should be the same for the Transformations
.
The way I would see the schedule:
ErrorMinimizer
would dictate what TransformationParameter
are used and thus which Transformation
needs to be applied.OK, here's a pull request for the simple check. https://github.com/ethz-asl/libpointmatcher/pull/165
The code for applying to a transformation to a cloud is the following: (https://github.com/ethz-asl/libpointmatcher/blob/master/pointmatcher/Transformation.cpp#L59)
//! Apply this chain to cloud, using parameters, mutates cloud template
void PointMatcher::Transformations::apply(DataPoints& cloud, const TransformationParameters& parameters) const
{
DataPoints transformedCloud;
for (TransformationsConstIt it = this->begin(); it != this->end(); ++it)
{
transformedCloud = (*it)->compute(cloud, parameters);
swapDataPoints(cloud, transformedCloud);
}
}
Please correct me if I am wrong. The idea here is that the iterator it point to the desired type of transform, be it similarity transform, or orthogonal transform, etc. Then, the transform itself, given by the "parameters" variable that is passed as input will be applied to the cloud.
There is a problem though. Nobody checks that there is exactly one iterator between this->begin() and this->end(). If there is more than one, the transform given in "parameters" will be applied to the cloud more than once, which is just wrong. I ran into this issue actually when in my case the set of iterators it was empty, and no transform was applied. That was hugely confusing.
In short, there must be a check here I think that there must be exactly one iterator between this->begin() and this->end().
I wonder if my interpretation is correct. If you agree with me, I will try to study this in more depth and put in a pull request (the check itself).