Open thyssentishman opened 4 days ago
@thyssentishman Hi, that is interesting, thanks. My best guess is that this happens because the filter uses std::srand
for seeding but rand()
(not std::rand()
) for drawing random numbers. Which compiler are you using? I wonder if something similar could happen in other PCL classes
For reference:
https://github.com/PointCloudLibrary/pcl/blob/master/test/filters/test_sampling.cpp#L148
https://github.com/PointCloudLibrary/pcl/blob/master/filters/include/pcl/filters/impl/random_sample.hpp#L66
https://github.com/PointCloudLibrary/pcl/blob/master/filters/include/pcl/filters/random_sample.h#L138
Hi @mvieth. Thanks for the quick reply. I was hoping that was the issue and applied your patch from #6179 (see below) with no success.
Index: filters/include/pcl/filters/random_sample.h
--- filters/include/pcl/filters/random_sample.h.orig
+++ filters/include/pcl/filters/random_sample.h
@@ -135,7 +135,7 @@ namespace pcl
inline float
unifRand ()
{
- return (static_cast<float>(rand () / static_cast<double>(RAND_MAX)));
+ return (static_cast<float>(std::rand () / static_cast<double>(RAND_MAX)));
//return (((214013 * seed_ + 2531011) >> 16) & 0x7FFF);
}
};
@@ -226,7 +226,7 @@ namespace pcl
inline float
unifRand ()
{
- return (static_cast<float> (rand () / static_cast<double>(RAND_MAX)));
+ return (static_cast<float> (std::rand () / static_cast<double>(RAND_MAX)));
}
};
}
Below the compiler I'm using:
OpenBSD clang version 16.0.6
Target: amd64-unknown-openbsd7.6
Thread model: posix
InstalledDir: /usr/bin
OpenBSD's rand(3) man page says that rand()
's underlying subsystem has been modified on OpenBSD to return non-deterministic values. Additionally the following is stated:
To satisfy portable code, srand() may be called to initialize the subsystem. In OpenBSD the seed variable is ignored, and strong random number results will be provided from arc4random(3). In other systems, the seed variable primes a simplistic deterministic algorithm.
Could this be the source of the issue?
@thyssentishman Oh okay. I was not aware that OpenBSD does that. Yes, that definitely sounds like it could cause this issue. std::srand
is probably just forwarded to srand
, and if srand
basically does nothing on OpenBSD, then it is clear that the test fails since it relies on the seed.
Honestly, to me it seems like a really bad idea for OpenBSD to intentionally break srand
. It completely screws up code portability and reproducibility of results. It is widely known that rand
is not a strong random number generator and that there are better alternatives both in C and C++, but why does OpenBSD not at least make srand
respect a seed given by the user (to ensure reproducibility/repeatability if desired), and if srand
was not called make rand
behave as if a random seed was set.
Sorry for the rant :smile:
I am wondering though if perhaps clang and gcc should forward std::srand
to srand_deterministic
on OpenBSD instead.
Regarding the RandomSample filter:
srand_deterministic
instead of std::srand
on OpenBSD. This would be quick and easy. It would be great if you could test whether this indeed makes the test pass@mvieth I understand and I agree that this is probably not good when one is expecting reproducibility of results (as we are experiencing here). It might be good if you could bring this up for discussion on OpenBSD's tech@ mailing lists if you like.
Regarding your proposed solutions, I was expecting srand_deterministic
to work, however the test #67
is still failing (whether or not I replace rand()
with std::rand()
).
Patch:
Index: filters/include/pcl/filters/impl/random_sample.hpp
--- filters/include/pcl/filters/impl/random_sample.hpp.orig
+++ filters/include/pcl/filters/impl/random_sample.hpp
@@ -63,7 +63,11 @@ pcl::RandomSample<PointT>::applyFilter (Indices &indic
removed_indices_->resize (N - sample_size);
// Set random seed so derived indices are the same each time the filter runs
+#ifdef __OpenBSD__
+ srand_deterministic (seed_);
+#else
std::srand (seed_);
+#endif
// Algorithm S
std::size_t i = 0;
@thyssentishman Can you try applying the same change at https://github.com/PointCloudLibrary/pcl/blob/master/filters/src/random_sample.cpp#L120 ?
@mvieth yep, that did it, thank you very much :) Would you like me to create a PR or would you amend #6179? Or should I just keep these patches local to the port?
100% tests passed, 0 tests failed out of 131
Just a quick note, I seem to be getting inconsistent results with test #103
(a_registration_test
) when running tests with multiple concurrent jobs e.g. make test -j8
. Are tests supposed to be thread safe?
@thyssentishman I can amend #6179
Just a quick note, I seem to be getting inconsistent results with test
#103
(a_registration_test
) when running tests with multiple concurrent jobs e.g.make test -j8
. Are tests supposed to be thread safe?
Yes, I think so. Do you have a log of a failure?
Yes, I think so. Do you have a log of a failure?
Sure, here is one:
103/131 Testing: a_registration_test
103/131 Test: a_registration_test
Command: "/usr/ports/pobj/pcl-1.14.1/build-amd64/test/registration/test_registration" "/usr/ports/pobj/pcl-1.14.1/pcl-pcl-1.14.1/test/bun0.pcd" "/usr/ports/pobj/pcl-1.14.1/pcl-pcl-1.14.1/test/bun4.pcd" "/usr/ports/pobj/pcl-1.14.1/pcl-pcl-1.14.1/test/milk_color.pcd"
Directory: /usr/ports/pobj/pcl-1.14.1/build-amd64/test/registration
"a_registration_test" start time: Nov 25 15:09 CET
Output:
----------------------------------------------------------
[==========] Running 13 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 13 tests from PCL
[ RUN ] PCL.findFeatureCorrespondences
[ OK ] PCL.findFeatureCorrespondences (0 ms)
[ RUN ] PCL.ICP_translated
/usr/ports/pobj/pcl-1.14.1/pcl-pcl-1.14.1/test/registration/test_registration.cpp:190: Failure
Expected: (icp.getFitnessScore()) < (1e-6), actual: 0.085357592720538375 vs 1e-06
/usr/ports/pobj/pcl-1.14.1/pcl-pcl-1.14.1/test/registration/test_registration.cpp:193: Failure
The difference between icp.getFinalTransformation()(0, 3) and 0.7 is 0.11399365663528438, which exceeds 2e-3, where
icp.getFinalTransformation()(0, 3) evaluates to 0.58600634336471558,
0.7 evaluates to 0.69999999999999996, and
2e-3 evaluates to 0.002.
/usr/ports/pobj/pcl-1.14.1/pcl-pcl-1.14.1/test/registration/test_registration.cpp:194: Failure
The difference between icp.getFinalTransformation()(1, 3) and 0.0 is 0.022719331085681915, which exceeds 2e-3, where
icp.getFinalTransformation()(1, 3) evaluates to -0.022719331085681915,
0.0 evaluates to 0, and
2e-3 evaluates to 0.002.
/usr/ports/pobj/pcl-1.14.1/pcl-pcl-1.14.1/test/registration/test_registration.cpp:195: Failure
The difference between icp.getFinalTransformation()(2, 3) and 0.0 is 0.089967794716358185, which exceeds 2e-3, where
icp.getFinalTransformation()(2, 3) evaluates to 0.089967794716358185,
0.0 evaluates to 0, and
2e-3 evaluates to 0.002.
[ FAILED ] PCL.ICP_translated (1 ms)
[ RUN ] PCL.IterativeClosestPoint
[ OK ] PCL.IterativeClosestPoint (4 ms)
[ RUN ] PCL.IterativeClosestPointWithNormals
[ OK ] PCL.IterativeClosestPointWithNormals (0 ms)
[ RUN ] PCL.IterativeClosestPointWithRejectors
[ OK ] PCL.IterativeClosestPointWithRejectors (106 ms)
[ RUN ] PCL.JointIterativeClosestPoint
[ OK ] PCL.JointIterativeClosestPoint (254 ms)
[ RUN ] PCL.IterativeClosestPointNonLinear
[ OK ] PCL.IterativeClosestPointNonLinear (41 ms)
[ RUN ] PCL.IterativeClosestPoint_PointToPlane
[pcl::IterativeClosestPoint::computeTransformation] Not enough correspondences found. Relax your threshold parameters.
[pcl::IterativeClosestPoint::computeTransformation] Not enough correspondences found. Relax your threshold parameters.
[pcl::IterativeClosestPoint::computeTransformation] Not enough correspondences found. Relax your threshold parameters.
[pcl::IterativeClosestPoint::computeTransformation] Not enough correspondences found. Relax your threshold parameters.
[pcl::IterativeClosestPoint::computeTransformation] Not enough correspondences found. Relax your threshold parameters.
[ OK ] PCL.IterativeClosestPoint_PointToPlane (6 ms)
[ RUN ] PCL.GeneralizedIterativeClosestPoint
[ OK ] PCL.GeneralizedIterativeClosestPoint (25 ms)
[ RUN ] PCL.GeneralizedIterativeClosestPointBFGS
[ OK ] PCL.GeneralizedIterativeClosestPointBFGS (51 ms)
[ RUN ] PCL.GeneralizedIterativeClosestPoint6D
[ OK ] PCL.GeneralizedIterativeClosestPoint6D (50 ms)
[ RUN ] PCL.PyramidFeatureHistogram
[ OK ] PCL.PyramidFeatureHistogram (1708 ms)
[ RUN ] PCL.PPFRegistration
[ OK ] PCL.PPFRegistration (3047 ms)
[----------] 13 tests from PCL (5299 ms total)
[----------] Global test environment tear-down
[==========] 13 tests from 1 test suite ran. (5300 ms total)
[ PASSED ] 12 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] PCL.ICP_translated
1 FAILED TEST
<end of output>
Test time = 5.41 sec
----------------------------------------------------------
Test Failed.
"a_registration_test" end time: Nov 25 15:09 CET
"a_registration_test" time elapsed: 00:00:05
----------------------------------------------------------
My first guess is that this is again related to random number generation, see here: https://github.com/PointCloudLibrary/pcl/blob/master/test/registration/test_registration.cpp#L169 ICP_translated is not tested with varying seeds, so the point cloud is always the same (except on OpenBSD). ICP is not a super stable algorithm, so I suppose it might fail for some random point clouds. I will try to come up with a solution.
@mvieth wouldn't seeding those rand()
calls with srand_deterministic()
be enought?
I have a finished port of pcl on OpenBSD and all tests are passing except the following one (apologies for the long code-block, for some reason the
<details>
block wasn't working):Any clues? I'm compiling pcl version 1.14.1