Bug of "meanValue" function

[Study-is-happy]

In "meanValue" function, if "mean" is assigned with a new value, one of the descriptor would change to this new value too. I don't know if it is what it should be, but I think the descriptor in this function should not be modified. Here is what I am guessing: when initializing the cluster, a random descriptor is chosen as the "mean", but not in a "clone" way. So if the mean is changing, that chosen descriptor would be changing at the same time (they are the referencing the same address)

[bcrobo]

Hey @Study-is-happy,

I'm not maintainer of this repository but still... I was intrigued by your suspicions. A bit of overview on FORB, FBrief and FSurf64 data types shows that they have respectively the following types:

FORB::TDescriptor -> cv::Mat
FORB::pDescriptor -> cv::Mat*

FBrief::TDescriptor -> std::bitset<L> // with L = 256
FBrief::pDescriptor -> std::bitset<L>*

FSurf64::TDescriptor -> std::vector<float>
FSurf64::pDescriptor -> std::vector<float>*

The method getFeatures of TemplatedVocabulary is building the vector of descriptors by taking the address of each plain type and push everything into a std::vector<pDescriptor>.

Later on, in HKmeansStep method, the clusters are declared as vector of plain types here and the F::meanValue(cluster_descriptors, clusters[c]); is called. But note that at this time:

clusters[c] -> cv::Mat // FORB
clusters[c] -> std::bitset<L> // FBrief
clusters[c] -> std::vector<float> // FSurb64

Lets have a look at each descriptor meanValue function. For FORB the line where you can have a doubt is probably:

mean = cv::Mat::zeros(1, FORB::L, CV_8U);

unfortunately you are right, mean points to a descriptor memory chunk. Since, it has the same size than the cv::Mat::zeros() it sets the memory chunk to zero and computes the mean value in-place (inside the descriptors population...).

For FBrief and FSurf64, the clusters[c] are plain std types. So it modifies the element of the clusters vector passed by reference (and has nothing to do with the input descriptors because the vector of clusters contains values (i.e. TDescriptor)). Note the descriptor in memory is still valid and is held by a cv::Mat header stored in the training_features you pass to create the vocabulary. In the end, I think the code should be fixed and harmonized to perform a copy at the cluster creation time (as it is done for FBrief and FSurf).

I agree this is not as readable as it should be, especially for the cv::Mat case.