[ml] Fix un-initialized centroids bug (k-means)

[Yuki992411]

I added a function that initializes centroids in kmeans.

[mvieth]

How about putting the for-loop inside computeCentroids, at line 98? That would in my opinion make logically the most sense: set centroids to zero - sum up points - divide by number of points. Otherwise the function computeCentroids would still have the requirement that the centroids are initialized to zero before calling the function.

[Yuki992411]

Thank you for your feedback.
Oh, I missed it. It's more natural to add initialization at line 98.
I made a code with low maintainability because I tried to reduce the number of initializations.
As you pointed out, I will correct it.

[mvieth]

Thanks!

[larshg]

Should we make a issue about creating a unit test?
Unless you, @Yuki992411, are up for the task to add a test?

[Yuki992411]

Thank you for your PR approval.
This is my first time to write test code.
However, it is a very good opportunity, so I will create a unit test myself and add it to issue #4569.
If you find any mistakes, please give me feedback.

[larshg]

You are missing:

/* ---[ */
int
main (int argc, char** argv)
{
  testing::InitGoogleTest (&argc, argv);
  return (RUN_ALL_TESTS ());
}

In the end of the test file.

[larshg]

And you can uses the short spdx version of the copyright:

/*
 * SPDX-License-Identifier: BSD-3-Clause
 *
 *  Point Cloud Library (PCL) - www.pointclouds.org
 *  Copyright (c) 2020-, Open Perception
 *
 *  All rights reserved
 */

beside the small changes, its looking good!

[larshg]

Looking good. Lets wait for CI's and @mvieth review on the unit test.
If no further changes are required, you can do a interactive rebase and set two of respective commits as fixups.

Then we have one commit with correction and one with the addition of unit test.

[Yuki992411]

OK!
Thank you for first review 😁

[mvieth]

Just two minor suggestions:

1. Switch the Case1/2 and Kmeans_computeCentroids. Usually it goes from general to specififc
2. We could also e.g. test if the number of computed centroids is as requested, i.e. 9 and 1.

[Yuki992411]

Thank you for your suggestion and review. @larshg @mvieth
In addition to the above suggestions, I have made some corrections.

• According to primer.md it was written that the TEST() arguments should not contain any underscores (_). So, I changed Kmeans_computeCentroids to ComputeCentroids.
• I fixed public indent at line 23.
• I summarized commits only in fix code and unit tests.

Please check my commit.

[mvieth]

Very nice, thanks again.
@larshg do you want to have a last look at the changes?

[larshg]

Looks ready 🚀