Change k =1 to k = 0 #5051
Change k =1 to k = 0 #5051
Conversation
Signed-off-by: ArkaprabhaChakraborty <[email protected]>
|
Are you also going to push new tests which show that the previous implementation was wrong, and your changes fix it? |
I need to check a new test. And then I can push the tests. By fixing I mean this bug was confirmed right :). So the testcase or use case where it triggered could be used. |
Signed-off-by: ArkaprabhaChakraborty <[email protected]>
test/filters/test_local_maximum.cpp
Outdated
| cloud_in.height = 4; | ||
| cloud_in.width = 3; | ||
| cloud_in.is_dense = true; | ||
| cloud_in.resize (6); |
There was a problem hiding this comment.
Why set the height and width if you're resizing?
There was a problem hiding this comment.
Also you resize to 6 points but you add 7 below 😄
There was a problem hiding this comment.
Oh yes :) bad mistake :)
There was a problem hiding this comment.
Why set the height and width if you're resizing?
I wanted to keep some similarity with the previous test :)
There was a problem hiding this comment.
Older tests are not modernised. Write modern code using the features added in PCL since then. Eg: transient_push_back for all non-final points, and push_back for the final point
Signed-off-by: ArkaprabhaChakraborty <[email protected]>
ArkaprabhaChakraborty
force-pushed
the
arkaprabha/fix-local-maximum
branch
from
906b3df to
387094b
Compare
| @@ -146,7 +146,7 @@ pcl::LocalMaximum<PointT>::applyFilterIndices (Indices &indices) | |||
|
|
|||
| // Check to see if a neighbor is higher than the query point | |||
| float query_z = (*input_)[iii].z; | |||
| for (std::size_t k = 1; k < radius_indices.size (); ++k) // k = 1 is the first neighbor | |||
| for (std::size_t k = 0; k < radius_indices.size (); ++k) // k = 1 is the first neighbor | |||
There was a problem hiding this comment.
Let's remove the bad comment completely.
Since we are now working with all indices, we can also use a range-for loop:
for (const auto& idx: radius_indices) {
if ((*input_)[idx].z > query.z) // 1st loop
point_is_visited[idx] = true; // 2nd loop
}|
|
||
| LocalMaximum<PointXYZ> lm; | ||
| lm.setInputCloud (cloud_in.makeShared ()); | ||
| lm.setRadius (1.0f); |
There was a problem hiding this comment.
Let's mark the points (via comments) which we expect to be removed
Signed-off-by: ArkaprabhaChakraborty <[email protected]>
Signed-off-by: ArkaprabhaChakraborty <[email protected]>
|
Looks like the build failed for something that is not my change :). |
|
Well nevermind :) |
|
@ArkaprabhaChakraborty Could you explain briefly what your new test does, that the existing test doesn't already cover? I ran your new test with the old version of |
Yes I need to adjust the tests. Also I need more info on this filter's working :) as I'm having some conceptual problems. So any links will do me good. |
I don't think that there is any paper, blog post, or similar describing the local maximum filter, but I assume you read the documentation? Paraphrasing that: for every point p, the filter searches the neighbours of p (all points within a radius), and compares the z-values of the neighbours with the z-value of p. If p has the highest z-value of all, it is removed |
I read the documentation. So I gotta figure out a test case which breaks the "older" Filter :) without the help of any blogs or such :). I'll report it soon. :) |
|
@mvieth @kunaltyagi sorry for such a late reply. I did try out some tests but none of them seems to "break" the older condition. :) |
Could you describe what kind of tests you tried out? Then maybe we can suggest something you missed |
Fixes #4999
Signed-off-by: ArkaprabhaChakraborty [email protected]