Skip to content

Unit test for MovingLeastSquares NaN output in certain conditions#788

Closed
translunar wants to merge 5 commits intoPointCloudLibrary:masterfrom
translunar:mls_debug
Closed

Unit test for MovingLeastSquares NaN output in certain conditions#788
translunar wants to merge 5 commits intoPointCloudLibrary:masterfrom
translunar:mls_debug

Conversation

@translunar
Copy link
Copy Markdown

This includes a test case which demonstrates #787. It's not a race condition after all, but a problem caused by the covariance computation method used for MovingLeastSquares.

I haven't removed the additional debugging message since this problem hasn't actually been fixed. In other words, this pull request probably isn't ready for a merge, but I wanted to make the unit test available.

@translunar translunar changed the title Mls debug Unit test for MovingLeastSquares NaN output in certain conditions Jul 4, 2014
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a special function to test if a point is finite: pcl::isFinite(pt).

@translunar
Copy link
Copy Markdown
Author

@taketwo Thanks for the feedback. I have corrected the test.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be PCL_ERROR as well.

@jspricke
Copy link
Copy Markdown
Member

Hi @MohawkJohn,

I've added some comments (there are some more style errors, I guess you will find them ;) ). Once you fix them, could you squash your commits? Then I'm fine with merging this.

@taketwo
Copy link
Copy Markdown
Member

taketwo commented Jul 31, 2014

I think we don't want to merge this. It exposes a bug, but does not fix it. If we merge it, we will get a test case which always fails.

@translunar
Copy link
Copy Markdown
Author

@taketwo Yeah, the goal here was to expose a bug, not to get this merged. I don't know how to address this particular problem, but thought I should spec it.

@SergioRAgostinho
Copy link
Copy Markdown
Member

The implementation of #1407 (comment) should fix this. I'll make sure this test is included then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: bug Type of issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants