Cleanup and improve unit test coverage for transformPointCloud functions #2245
Conversation
* Remove dependency on features module * Remove global variables and constants * Remove using namespace std * Remove commented out code * Remove redundant .points
The demeanPointCloud() function belongs to the centroid.h header, thus moving the unit test to the corresponding test suite. As a consequence, transforms tests no longer depend on pcl_io.
The getTransFromUnitVectorsZY/XY() functions belong to the eigen.h header, thus moving the unit test to the corresponding test suite.
Besides from testing transformPoint() and transformPointCloud(), this case also tests getTransformation() and getTranslationAndEulerAngles() that belong to a different header (eigen.h). Therefore, moved to appropriate test suite.
SergioRAgostinho
added
needs: code review
There was a problem hiding this comment.
I started making a number of comments in full review spirit to only realize later you were just moving code around. I would propose we still have a look at these if you don't mind.
The code you actually added on your own is good to go on my side.
Edit: Since I only realized this (the moving of code) halfway through the review, I stopped looking at code which was not originally yours. If you feel like modifying the stuff moved, I'll have another look.
| @@ -1,7 +1,7 @@ | |||
| set(SUBSYS_NAME tests_common) | |||
| set(SUBSYS_DESC "Point cloud library common module unit tests") | |||
| PCL_SET_TEST_DEPENDENCIES(SUBSYS_DEPS common) | |||
| set(OPT_DEPS io features search kdtree octree) | |||
| set(OPT_DEPS io search kdtree octree) | |||
| EXPECT_NEAR (mat_demean (0, cloud_demean.size () - 1), -0.048849, 1e-4); | ||
| EXPECT_NEAR (mat_demean (1, cloud_demean.size () - 1), 0.072507, 1e-4); | ||
| EXPECT_NEAR (mat_demean (2, cloud_demean.size () - 1), -0.071702, 1e-4); | ||
| } |
There was a problem hiding this comment.
Consider splitting these into 4 individual tests with a common fixture?
TEST_F (demeanPointCloud, PointCloud)
TEST_F (demeanPointCloud, PointCloudIndices)
TEST_F (demeanPointCloud, EigenMat)
TEST_F (demeanPointCloud, EigenMatIndices)
test/common/test_centroid.cpp
Outdated
| EXPECT_EQ (cloud_demean.width, cloud.width); | ||
| EXPECT_EQ (cloud_demean.height, cloud.height); | ||
| EXPECT_EQ (cloud_demean.size (), cloud.size ()); | ||
|
|
There was a problem hiding this comment.
What are we expecting to happen to the previously set sensor origin and orientation?
test/common/test_centroid.cpp
Outdated
| EXPECT_NEAR (cloud_demean[cloud_demean.size () - 1].z, -0.071702, 1e-4); | ||
|
|
||
| std::vector<int> indices (cloud.size ()); | ||
| for (int i = 0; i < static_cast<int> (indices.size ()); ++i) { indices[i] = i; } |
There was a problem hiding this comment.
Wouldn't it be better to try a case in which not all indices are used? Just the first and the last for instance?
test/common/test_centroid.cpp
Outdated
| EXPECT_NEAR (cloud_demean[0].x, 0.034503, 1e-4); | ||
| EXPECT_NEAR (cloud_demean[0].y, 0.010837, 1e-4); | ||
| EXPECT_NEAR (cloud_demean[0].z, 0.013447, 1e-4); | ||
|
|
There was a problem hiding this comment.
We should check that cloud_demean[0].w is preserved as 1, even after the demean.
There was a problem hiding this comment.
Firstly, there is no .w member, only data[3]. Secondly, is it the part of the contract that this padding number is always 1.0? I'm not sure I ever seen this stated explicitly anywhere.
There was a problem hiding this comment.
Secondly, is it the part of the contract that this padding number is always 1.0?
I makes sense to initialize by default with 1.0, but from then onwards, we can't assume anything anymore.
I'm not really sure how to handle this. In a sense, in most cases we're likely to have the homogeneous coordinate set to the same value in all points. But if it's not, I don't think demeanPointCloud is even handling it. So yeah, you've got a point.
| std::cerr << "No test file given. Please download `bun0.pcd` and pass its path to the test." << std::endl; | ||
| return (-1); | ||
| } | ||
| if (io::loadPCDFile (argv[1], cloud_blob) < 0) |
There was a problem hiding this comment.
I would prefer to avoid having to load bunny from a PCD file. It would remove io as an optional dependency for these tests.
Why not populate randomly a 10x3 Eigen matrix and use that as a data source?
SergioRAgostinho
added
needs: author reply
|
Thanks for taking time to review. I pushed one small commit addressing one of the points you made. I fully agree with all other, but I don't have cycles now to clean up messy unit tests :( (I guess you know where I am heading with this PR. To be honest my "ok, let's just try to use Eigen multiplication for transforms" project took way more effort than I anticipated.) |
Classic :) I believe anything less of what you made you leave room for doubt and questioning. Now we're able to quickly benchmark multiple configurations and settings and easily interpret the results. Also, if you think about it, you just wrote a boilerplate for whatever benchmark you might need to do in the future. To be honest, for my future ones as well ^^ It should pay off in the long run. |
No description provided.