Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_kfpcs_ia fails due to missing file(s) #1227

Closed
taketwo opened this issue May 5, 2015 · 19 comments
Closed

test_kfpcs_ia fails due to missing file(s) #1227

taketwo opened this issue May 5, 2015 · 19 comments

Comments

@taketwo
Copy link
Member

taketwo commented May 5, 2015

@theilerp Could you please have a look? The test uses some files that are not present in the repository.

@theilerp
Copy link
Contributor

theilerp commented May 5, 2015

They got lost somehow during squashing, commting, pulling chaos. I have them here, what is the easiest way to give them to you?

@taketwo
Copy link
Member Author

taketwo commented May 5, 2015

Submit a pull request, of course :) Are they heavy? If this is the case perhaps it's better to reuse some of the test files already available in the repo.

@theilerp
Copy link
Contributor

theilerp commented May 5, 2015

of course :). No they are not heavy, both are around 20kb ^^

@theilerp
Copy link
Contributor

theilerp commented May 5, 2015

done!

@taketwo
Copy link
Member Author

taketwo commented May 5, 2015

Travis did not reach the test, but on my computer it fails:

83: [ RUN      ] PCL.KFPCSInitialAlignment
83: /media/Workspace/Libraries/pcl/test/registration/test_kfpcs_ia.cpp:87: Failure
83: The difference between angle3d and 0.f is 1.5706658363342285, which exceeds 0.1745f, where
83: angle3d evaluates to 1.5706658363342285,
83: 0.f evaluates to 0, and
83: 0.1745f evaluates to 0.1745000034570694.
83: [  FAILED  ] PCL.KFPCSInitialAlignment (562 ms)
83: [----------] 1 test from PCL (562 ms total)

@theilerp
Copy link
Contributor

theilerp commented May 5, 2015

Waaaaait, this looks like the test is also based on an old version, what the hell did I do with the squashing.

I have to verify this, but it could be, that the test also verifies the result of the registration and because it is based on random sampling, it can happen that the registration fails (although I am quite sure that the error rate was somewhere arount 1%, so you were anyway quite unlucky :-), try to rerun it).

Because this has nothing to do with the applicability of the algorithm, I removed the verifaction of the result in the test in a later version.

@theilerp
Copy link
Contributor

theilerp commented May 5, 2015

Okay it is as expected, the two lines

EXPECT_NEAR (angle3d, 0.f, 0.1745f); // 10°
EXPECT_NEAR (translation3d, 0.f, 1.f); // 1m

are not commented anymore.

I could do that, but the side effect is, that the test then only checks if the aligned point cloud has the same amount of points as the input one, which is already garanteed after initialization. On the other hand, it does not make sense to check the result, because as we have just seen, it can randomly be wrong.

Any suggestions?

@taketwo
Copy link
Member Author

taketwo commented May 6, 2015

although I am quite sure that the error rate was somewhere arount 1%, so you were anyway quite unlucky :-)

On my machine the test consistently fails. I ran it about 20 times and it didn't succeed any single time. The difference in angle was always at least 1.5, and sometimes as large as 3.2.

@theilerp
Copy link
Contributor

theilerp commented May 6, 2015

Hm, only explanation from my side is, that I used a wrong (e.g. inversed) transformation matrix as groundtruth. I will verify that.

But in any case: does it make sense to restrict the test success of a random based method in such a way, that it can fail although it ran as it is supposed to?

In that case I would correct the groundtruth but also remove the 'EXPECT_NEAR' parts...

@taketwo
Copy link
Member Author

taketwo commented May 6, 2015

Good question. You may check how this is handled in other randomized algorithms in PCL. One obvious option would be to allow several runs. If you give guarantee that it fails only 1% of the time, then we are 99.99% confident that it will succeed at least once in two runs.

@theilerp
Copy link
Contributor

theilerp commented May 6, 2015

Okay found the problem, to summarize:

  • I used the correct groundtruth, but Successrate was much lower than 99% (but on my computer it was still somewhere around 90%, pretty interesting)
  • Found the "bug" in test_kfpcs_ia_data.h, which caused the algorithm to early terminate after 1 try which naturally increases the error rate
  • I still followed your suggestion and added a loop of 2 independent alignments to check the success.

I will do a new pull request as soon as I finished my testing (100 runs on my computer). Can you check it then again on your machine?

@taketwo
Copy link
Member Author

taketwo commented May 6, 2015

Sure, just post a link to the feature branch on your fork.

@theilerp
Copy link
Contributor

theilerp commented May 6, 2015

I am not very familiar with git (as you definitely have found out over the last half year), not sure if this link is what you need:

https://github.com/theilerp/pcl/tree/theilerp-kfpcs-testfiles

Otherwise I would just do another pull request.

@taketwo
Copy link
Member Author

taketwo commented May 6, 2015

Yes, this is it. I will give it a try during the day.

@taketwo
Copy link
Member Author

taketwo commented May 6, 2015

Takes forever (up to 23 seconds) and fails in the end, consistently.

[ RUN      ] PCL.KFPCSInitialAlignment
/home/sergey/Workspace/Libraries/pcl-patches/test/registration/test_kfpcs_ia.cpp:91: Failure
The difference between angle3d and 0.f is 1.5709066390991211, which exceeds 0.1745f, where
angle3d evaluates to 1.5709066390991211,
0.f evaluates to 0, and
0.1745f evaluates to 0.1745000034570694.
[  FAILED  ] PCL.KFPCSInitialAlignment (17195 ms)

@theilerp
Copy link
Contributor

theilerp commented May 6, 2015

This is definitely veeery strange. Why the hell is it working for me on my machine???
I will check some more things, but I am really confused now...

The 23seconds is normal, in case you do not use multi-threading with openmp, otherwise it should be much faster.

@theilerp
Copy link
Contributor

theilerp commented May 6, 2015

Stupid me, because I didn't use the same groundtruth ^^.

I found the error, the groundtruth had a typing error (-0.999 instead of 0.999).

Next commit should hopefully work, I am very sorry for the inconvenience

@taketwo
Copy link
Member Author

taketwo commented May 6, 2015

Works on my side now. But don't forget to remove the commit that comments out the angle check.

Also can we skip next iterations after successful alignment? Tests often don't have enough time to run on Travis, so adding another 20 seconds is not so good.

@theilerp
Copy link
Contributor

theilerp commented May 6, 2015

Done and pull request --> #1229

@taketwo taketwo closed this as completed May 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants