Better performance, error handling and other improvements in SAC classes #3642

mvieth · 2020-02-10T20:07:02Z

No description provided.

kunaltyagi · 2020-02-11T17:09:11Z

Might be better to not modify the public API (or add overloads instead of replacing the original functions)

mvieth · 2020-02-11T20:04:04Z

I could add adapters for getSamples, computeModelCoefficients, isSampleGood, and the other functions, but I would mark them as deprecated right away.

kunaltyagi · 2020-02-12T00:24:14Z

Might not, because based on feedback on RFC-002, we might make pcl::index_t be either 32 bit or 64 bit at build time. The deprecation will need to be undone in case someone is building pcl::index_t in 32-bit mode (which might be the default)

mvieth · 2020-02-15T10:41:17Z

I wasn't aware that there are RFCs in the wiki. I thought this kind of stuff is discussed in issues. I made a comment in the issue linked in the RFC.

kunaltyagi · 2020-02-15T10:58:22Z

I thought this kind of stuff is discussed in issues

It is. I created RFC since it turns out a fleshing out a topic is hard to do in an issue. RFC allows people to comment and discuss in the relevant gitter rooms without creating noise for other non-interested people.

mvieth · 2020-02-15T11:06:53Z

Well, nobody is forced to read an issue or its comments. As long as an issue is confined to one topic, I see no problem with discussing things that way. I just wanted to note that many people may not know about the RFCs in the wiki and thus don't participate in the discussion.

kunaltyagi

Is there a reason behind changing the behavior of isModelValid?

sample_consensus/include/pcl/sample_consensus/impl/sac_model_circle3d.hpp

sample_consensus/include/pcl/sample_consensus/impl/sac_model_cone.hpp

mvieth · 2020-02-16T08:55:09Z

Is there a reason behind changing the behavior of isModelValid?

What do you mean? Where did I do that?

kunaltyagi · 2020-02-16T12:47:31Z

You deleted the pcl::SampleConsensusModelNormalSphere<PointT, PointNT>::isModelValid function which effectively changes the behavior of isModelValid since it defaults to the base-class function

mvieth · 2020-02-16T12:51:51Z

I added a using SampleConsensusModelSphere<PointT>::isModelValid; in sac_model_normal_sphere.h, and that one looks exactly like the function I deleted. So the behaviour should be the same, no?

The number of model coefficients is checked there

- Add braces for clarity - Move n_inliers_count to where it is used - Remove initialization of d_cur_penalty because that value is overwritten, thus never read

kunaltyagi · 2020-02-16T14:22:42Z

PS: I started work on a pcl::index_t and I'll create a PR soon. May take a day or two. Could you wait for that PR to merge so you can use that instead of breaking the public API.

kunaltyagi

Most of the PR LGTM. I'll take a deeper look after the weekend.

sample_consensus/include/pcl/sample_consensus/impl/sac_model_circle.hpp

sample_consensus/include/pcl/sample_consensus/impl/lmeds.hpp

kunaltyagi

If it's possible, I'd remove the changes wrt sort, partition, nth_element from this PR so we can highlight them in the change-log separately.

sample_consensus/include/pcl/sample_consensus/impl/lmeds.hpp

sample_consensus/include/pcl/sample_consensus/impl/sac_model_circle.hpp

SergioRAgostinho · 2020-03-10T20:35:15Z

@kunaltyagi Ok, I think I applied all of your suggestions. But what is going on with the MacOS checks?

Broken build tools environment in CI.

kunaltyagi

Overall, LGTM. I have 2 minor comments. Sergio is correct regarding the CI. Apples like to break stuff.

Thanks a lot for this @mvieth. Parts of the patch are awesome ❤️

sample_consensus/include/pcl/sample_consensus/impl/lmeds.hpp

SergioRAgostinho

LGTM. Thanks 👍 Before I give the green light, here are some things which raised my eyebrow.

Important things:

Not a consequence from this PR, but the inliers checks look fishy. But I might be overlooking something. See inline comments.

Commits that should be squashed after the entire review process is complete
3201899
0ad9e94
8bc774a
818c07c

Minor things:

This commit 8d68a6d is a style preference that I do not agree with but I don't mind when one is the code author. For the future, don't submit commits with just these types of changes unless the code is really cryptic. Ultimately everything is going to be formatted with clang-format at some point, so the style is going to be unified there.

Future work (?):

Commit 78760f0 is asking for for-range loop replacements on those index loops.

sample_consensus/include/pcl/sample_consensus/impl/sac_model_circle3d.hpp

sample_consensus/include/pcl/sample_consensus/impl/sac_model_sphere.hpp

sample_consensus/include/pcl/sample_consensus/impl/sac_model_stick.hpp

sample_consensus/include/pcl/sample_consensus/impl/sac_model_cylinder.hpp

sample_consensus/include/pcl/sample_consensus/impl/sac_model_stick.hpp

sample_consensus/include/pcl/sample_consensus/sac_model_normal_sphere.h

sample_consensus/include/pcl/sample_consensus/impl/lmeds.hpp

New code has been added

sample_consensus/include/pcl/sample_consensus/impl/sac_model_circle.hpp

sample_consensus/include/pcl/sample_consensus/impl/sac_model_circle3d.hpp

sample_consensus/include/pcl/sample_consensus/impl/sac_model_sphere.hpp

sample_consensus/include/pcl/sample_consensus/impl/sac_model_stick.hpp

SergioRAgostinho

LGTM. Thanks. Squash those commits I mentioned early and it's good to merge.

- If there are NaN values in distances, sorting does not work. So move all NaN value to the end and continue with the valid sub range. erase-ing them could also be an option, but that might be slower. - When searching for the median, it is much faster to use nth_element instead of sort-ing the entire array. My benchmarks suggest that this can make computeModel 4 times faster

mvieth · 2020-03-19T09:00:06Z

Done. Let me know if there are more commits you would like me to squash.

kunaltyagi

LGTM. Nice commits. I don't feel any further reorganization is needed

mvieth force-pushed the sample_consensus branch from 9ccc1f7 to 9c05326 Compare February 11, 2020 08:17

kunaltyagi added the module: sample_consensus label Feb 11, 2020

kunaltyagi reviewed Feb 16, 2020

View reviewed changes

sample_consensus/include/pcl/sample_consensus/impl/sac_model_circle3d.hpp Outdated Show resolved Hide resolved

sample_consensus/include/pcl/sample_consensus/impl/sac_model_cone.hpp Outdated Show resolved Hide resolved

mvieth added 8 commits February 16, 2020 14:25

Add curly braces to make code more clear

8d68a6d

Use model_size_ and sample_size_ more consistently

0c248b6

Use floating point literals more explicitly

0c6b169

Use push_back in selectWithinDistance

78760f0

Add checks for sample size to isSampleGood functions

98a974d

Use isModelValid consistently

480fbf3

The number of model coefficients is checked there

Use isModelValid from parent class

07a71a4

Make minor improvements to lmeds

ec9d2fe

- Add braces for clarity - Move n_inliers_count to where it is used - Remove initialization of d_cur_penalty because that value is overwritten, thus never read

mvieth force-pushed the sample_consensus branch from 62bbf45 to 3201899 Compare February 16, 2020 13:30

mvieth marked this pull request as ready for review February 22, 2020 09:06

mvieth requested a review from kunaltyagi February 22, 2020 09:07

kunaltyagi reviewed Feb 22, 2020

View reviewed changes

sample_consensus/include/pcl/sample_consensus/impl/sac_model_circle.hpp Show resolved Hide resolved

sample_consensus/include/pcl/sample_consensus/impl/lmeds.hpp Show resolved Hide resolved

kunaltyagi added effort: medium Rough estimate of time needed to fix/implement/solve effort: high Rough estimate of time needed to fix/implement/solve and removed effort: medium Rough estimate of time needed to fix/implement/solve labels Feb 24, 2020

kunaltyagi requested changes Feb 24, 2020

View reviewed changes

kunaltyagi changed the title ~~Improve module sample consensus~~ [sample consensus] Better error handling and other improvements Feb 24, 2020

kunaltyagi added the changelog: enhancement Meta-information for changelog generation label Mar 11, 2020

kunaltyagi changed the title ~~[sample consensus] Better error handling and other improvements~~ [sample consensus] Better performance, error handling and other improvements Mar 11, 2020

kunaltyagi reviewed Mar 11, 2020

View reviewed changes

sample_consensus/include/pcl/sample_consensus/impl/lmeds.hpp Show resolved Hide resolved

sample_consensus/include/pcl/sample_consensus/impl/lmeds.hpp Show resolved Hide resolved

kunaltyagi previously approved these changes Mar 11, 2020

View reviewed changes

SergioRAgostinho self-requested a review March 11, 2020 10:26

SergioRAgostinho added needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels Mar 11, 2020

SergioRAgostinho reviewed Mar 14, 2020

View reviewed changes

SergioRAgostinho self-requested a review March 17, 2020 11:55

SergioRAgostinho reviewed Mar 18, 2020

View reviewed changes

SergioRAgostinho approved these changes Mar 18, 2020

View reviewed changes

SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Mar 18, 2020

mvieth added 4 commits March 19, 2020 09:54

Change PCL_DEBUG to PCL_ERROR

3a0225b

Simplify isModelValid for cylinder and cone

d31dae6

Fix inliers checks in optimizeModelCoefficients

eb3a8de

mvieth force-pushed the sample_consensus branch from 30599f2 to eb3a8de Compare March 19, 2020 08:55

kunaltyagi approved these changes Mar 19, 2020

View reviewed changes

kunaltyagi removed the needs: author reply Specify why not closed/merged yet label Mar 19, 2020

kunaltyagi requested a review from SergioRAgostinho March 19, 2020 10:18

kunaltyagi added the needs: code review Specify why not closed/merged yet label Mar 19, 2020

SergioRAgostinho approved these changes Mar 19, 2020

View reviewed changes

SergioRAgostinho merged commit 55fc3fa into PointCloudLibrary:master Mar 19, 2020

taketwo removed the needs: code review Specify why not closed/merged yet label Mar 19, 2020

taketwo changed the title ~~[sample consensus] Better performance, error handling and other improvements~~ Better performance, error handling and other improvements in SAC classes Mar 19, 2020

mvieth deleted the sample_consensus branch March 21, 2020 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better performance, error handling and other improvements in SAC classes #3642

Better performance, error handling and other improvements in SAC classes #3642

mvieth commented Feb 10, 2020

kunaltyagi commented Feb 11, 2020

mvieth commented Feb 11, 2020

kunaltyagi commented Feb 12, 2020

mvieth commented Feb 15, 2020

kunaltyagi commented Feb 15, 2020

mvieth commented Feb 15, 2020

kunaltyagi left a comment

mvieth commented Feb 16, 2020

kunaltyagi commented Feb 16, 2020

mvieth commented Feb 16, 2020

kunaltyagi commented Feb 16, 2020

kunaltyagi left a comment

kunaltyagi left a comment

SergioRAgostinho commented Mar 10, 2020

kunaltyagi left a comment

SergioRAgostinho left a comment •

edited

Loading

SergioRAgostinho left a comment

mvieth commented Mar 19, 2020

kunaltyagi left a comment

Better performance, error handling and other improvements in SAC classes #3642

Better performance, error handling and other improvements in SAC classes #3642

Conversation

mvieth commented Feb 10, 2020

kunaltyagi commented Feb 11, 2020

mvieth commented Feb 11, 2020

kunaltyagi commented Feb 12, 2020

mvieth commented Feb 15, 2020

kunaltyagi commented Feb 15, 2020

mvieth commented Feb 15, 2020

kunaltyagi left a comment

Choose a reason for hiding this comment

mvieth commented Feb 16, 2020

kunaltyagi commented Feb 16, 2020

mvieth commented Feb 16, 2020

kunaltyagi commented Feb 16, 2020

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

SergioRAgostinho commented Mar 10, 2020

kunaltyagi left a comment

Choose a reason for hiding this comment

SergioRAgostinho left a comment • edited Loading

Choose a reason for hiding this comment

SergioRAgostinho left a comment

Choose a reason for hiding this comment

mvieth commented Mar 19, 2020

kunaltyagi left a comment

Choose a reason for hiding this comment

SergioRAgostinho left a comment •

edited

Loading