Refactor multigrid_ndt_omp #32

anhnv3991 · 2024-03-07T14:15:39Z

Refactored multigrid_ndt_omp.h and multigrid_ndt_omp_impl.hpp to remove unused variables and functions and prepare for the parallel ndt build (which will be added in the future).

Signed-off-by: anhnv3991 <[email protected]>

SakodaShintaro · 2024-03-11T10:35:59Z

@anhnv3991
There seems to be a difference with the previous results, so I would like to find out the cause.
https://github.com/tier4/ndt_omp/actions/runs/8229606913/job/22501142433

If possible, I think it's a good idea to submit only pure refactoring parts where there are no differences in the results as a separate pull request. 🦭

anhnv3991 · 2024-03-11T13:52:58Z

@SakodaShintaro I guess the difference is caused by the change from float to double. I'll remove float to double change tomorrow.

Signed-off-by: Shintaro Sakoda <[email protected]>

SakodaShintaro

The difference has become very small. Good

I'm concerned that it's just a little slower, but I think it's OK to merge.

I have left some comments and would appreciate it if you could respond.

include/multigrid_pclomp/multigrid_ndt_omp.h

include/multigrid_pclomp/multigrid_ndt_omp_impl.hpp

SakodaShintaro · 2024-03-13T08:08:55Z

include/multigrid_pclomp/multigrid_ndt_omp_impl.hpp

@@ -681,7 +645,7 @@ void pclomp::MultiGridNormalDistributionsTransform<PointSource, PointTarget>::co

 //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
 template<typename PointSource, typename PointTarget>
-void pclomp::MultiGridNormalDistributionsTransform<PointSource, PointTarget>::updateHessian(Eigen::Matrix<double, 6, 6> &hessian, const Eigen::Matrix<double, 3, 6> &point_gradient, const Eigen::Matrix<double, 18, 6> &point_hessian, const Eigen::Vector3d &x_trans, const Eigen::Matrix3d &c_inv) const {
+void pclomp::MultiGridNormalDistributionsTransform<PointSource, PointTarget>::updateHessian(Eigen::Matrix<double, 6, 6> &hessian, const Eigen::Matrix<double, 4, 6> &point_gradient, const Eigen::Matrix<double, 24, 6> &point_hessian, const Eigen::Vector3d &x_trans, const Eigen::Matrix3d &c_inv) const {


Please let me confirm my understanding.
updateHessian is only used from computeHessian, and computeHessian is only used in the following lines.

ndt_omp/include/multigrid_pclomp/multigrid_ndt_omp_impl.hpp

Line 937 in 01ae242

if(step_iterations) computeHessian(hessian, trans_cloud, x_t);

This is a branch when something is updated in line_search, and since line_search is currently turned off, step_iteration is always 0, so I don't think computeHessian will be called.

Is my understanding correct?
In other words, even if there is a bug in updateHessian, it will not be noticed in its current behavior, and I am concerned that it will occur someday when line_search is turned on.

@SakodaShintaro If the line search is turned on, errors will 100% occur. It is because the angular derivatives (j_ang_a_, j_ang_b, etc) are not pre-computed. Those variables are used by the computeHessian.

@anhnv3991
I would like to maintain line_search to be able to use.

Steps

(1) Organize parameters: this PR #43
(2) Add a bool type parameter called use_line_search and make it possible to switch true/false: next PR #46
(3) I would like to merge this PR while maintaining a buildable state.

Signed-off-by: anhnv3991 <[email protected]>

* Organized ndt params Signed-off-by: Shintaro Sakoda <[email protected]> * style(pre-commit): autofix --------- Signed-off-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]>

* Coding Signed-off-by: anhnv3991 <[email protected]> * Rearrange the code to avoid duplicated processing Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Updated reference/result.csv Signed-off-by: Shintaro Sakoda <[email protected]> * A minor fix Signed-off-by: Shintaro Sakoda <[email protected]> * refactor: organized ndt params (#43) * Organized ndt params Signed-off-by: Shintaro Sakoda <[email protected]> * style(pre-commit): autofix --------- Signed-off-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> * Coding Signed-off-by: anhnv3991 <[email protected]> * Fixed conflicts after rebasing Signed-off-by: anhnv3991 <[email protected]> * Coding Signed-off-by: anhnv3991 <[email protected]> * Coding Signed-off-by: anhnv3991 <[email protected]> * Fixed some bugs Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Fixed to run workflow Signed-off-by: Shintaro Sakoda <[email protected]> --------- Signed-off-by: anhnv3991 <[email protected]> Signed-off-by: Shintaro Sakoda <[email protected]> Co-authored-by: anhnv3991 <[email protected]> Co-authored-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> Co-authored-by: SakodaShintaro <[email protected]>

* Added tp_score to regression_test Signed-off-by: Shintaro Sakoda <[email protected]> * Updated result.csv by result of GitHub Actions Signed-off-by: Shintaro Sakoda <[email protected]> * Updated by the latest result Signed-off-by: Shintaro Sakoda <[email protected]> --------- Signed-off-by: Shintaro Sakoda <[email protected]>

* Added a parameter "use_line_search" Signed-off-by: Shintaro Sakoda <[email protected]> * Added default value use_line_search=false Signed-off-by: Shintaro Sakoda <[email protected]> --------- Signed-off-by: Shintaro Sakoda <[email protected]>

Signed-off-by: anhnv3991 <[email protected]>

Signed-off-by: Shintaro Sakoda <[email protected]>

Signed-off-by: anhnv3991 <[email protected]>

Signed-off-by: Shintaro SAKODA <[email protected]>

SakodaShintaro

Looks Good To Me

I apologize for making you wait for such a long time and for causing so many conflicts.

Final result

anhnv3991 · 2024-03-19T10:06:16Z

@SakodaShintaro Thanks a lot!

* Refactoring multigrid_ndt_omp Signed-off-by: anhnv3991 <[email protected]> * Refactored multigrid_ndt_omp Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Fixed to build Signed-off-by: Shintaro Sakoda <[email protected]> * Removed line breaks Signed-off-by: Shintaro Sakoda <[email protected]> * For reference, this should be discared later Signed-off-by: anhnv3991 <[email protected]> * Testing Signed-off-by: anhnv3991 <[email protected]> * Clean up a bit Signed-off-by: anhnv3991 <[email protected]> * Clean up a bit Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * refactor: organized ndt params (#43) * Organized ndt params Signed-off-by: Shintaro Sakoda <[email protected]> * style(pre-commit): autofix --------- Signed-off-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> * Speed up multigrid NDT align (#41) * Coding Signed-off-by: anhnv3991 <[email protected]> * Rearrange the code to avoid duplicated processing Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Updated reference/result.csv Signed-off-by: Shintaro Sakoda <[email protected]> * A minor fix Signed-off-by: Shintaro Sakoda <[email protected]> * refactor: organized ndt params (#43) * Organized ndt params Signed-off-by: Shintaro Sakoda <[email protected]> * style(pre-commit): autofix --------- Signed-off-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> * Coding Signed-off-by: anhnv3991 <[email protected]> * Fixed conflicts after rebasing Signed-off-by: anhnv3991 <[email protected]> * Coding Signed-off-by: anhnv3991 <[email protected]> * Coding Signed-off-by: anhnv3991 <[email protected]> * Fixed some bugs Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Fixed to run workflow Signed-off-by: Shintaro Sakoda <[email protected]> --------- Signed-off-by: anhnv3991 <[email protected]> Signed-off-by: Shintaro Sakoda <[email protected]> Co-authored-by: anhnv3991 <[email protected]> Co-authored-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> * fix: add tp to regression test (#44) * Added tp_score to regression_test Signed-off-by: Shintaro Sakoda <[email protected]> * Updated result.csv by result of GitHub Actions Signed-off-by: Shintaro Sakoda <[email protected]> * Updated by the latest result Signed-off-by: Shintaro Sakoda <[email protected]> --------- Signed-off-by: Shintaro Sakoda <[email protected]> * fix: add a param use line search (#46) * Added a parameter "use_line_search" Signed-off-by: Shintaro Sakoda <[email protected]> * Added default value use_line_search=false Signed-off-by: Shintaro Sakoda <[email protected]> --------- Signed-off-by: Shintaro Sakoda <[email protected]> * Refactoring multigrid_ndt_omp Signed-off-by: anhnv3991 <[email protected]> * Refactored multigrid_ndt_omp Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Removed line breaks Signed-off-by: Shintaro Sakoda <[email protected]> * For reference, this should be discared later Signed-off-by: anhnv3991 <[email protected]> * Testing Signed-off-by: anhnv3991 <[email protected]> * Clean up a bit Signed-off-by: anhnv3991 <[email protected]> * Clean up a bit Signed-off-by: anhnv3991 <[email protected]> * Debugging Signed-off-by: anhnv3991 <[email protected]> * Fixed a bug Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Fixed build error Signed-off-by: anhnv3991 <[email protected]> * Updated result.csv Signed-off-by: Shintaro SAKODA <[email protected]> --------- Signed-off-by: anhnv3991 <[email protected]> Signed-off-by: Shintaro Sakoda <[email protected]> Signed-off-by: Shintaro SAKODA <[email protected]> Co-authored-by: anhnv3991 <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> Co-authored-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]>

* Coding Signed-off-by: anhnv3991 <[email protected]> * Rearrange the code to avoid duplicated processing Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Added multithread voxel grid build Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Fixed Signed-off-by: anhnv3991 <[email protected]> * Fixed wrong tmp_score usage Signed-off-by: anhnv3991 <[email protected]> * Refactor multigrid_ndt_omp (#32) * Refactoring multigrid_ndt_omp Signed-off-by: anhnv3991 <[email protected]> * Refactored multigrid_ndt_omp Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Fixed to build Signed-off-by: Shintaro Sakoda <[email protected]> * Removed line breaks Signed-off-by: Shintaro Sakoda <[email protected]> * For reference, this should be discared later Signed-off-by: anhnv3991 <[email protected]> * Testing Signed-off-by: anhnv3991 <[email protected]> * Clean up a bit Signed-off-by: anhnv3991 <[email protected]> * Clean up a bit Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * refactor: organized ndt params (#43) * Organized ndt params Signed-off-by: Shintaro Sakoda <[email protected]> * style(pre-commit): autofix --------- Signed-off-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> * Speed up multigrid NDT align (#41) * Coding Signed-off-by: anhnv3991 <[email protected]> * Rearrange the code to avoid duplicated processing Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Updated reference/result.csv Signed-off-by: Shintaro Sakoda <[email protected]> * A minor fix Signed-off-by: Shintaro Sakoda <[email protected]> * refactor: organized ndt params (#43) * Organized ndt params Signed-off-by: Shintaro Sakoda <[email protected]> * style(pre-commit): autofix --------- Signed-off-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> * Coding Signed-off-by: anhnv3991 <[email protected]> * Fixed conflicts after rebasing Signed-off-by: anhnv3991 <[email protected]> * Coding Signed-off-by: anhnv3991 <[email protected]> * Coding Signed-off-by: anhnv3991 <[email protected]> * Fixed some bugs Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Fixed to run workflow Signed-off-by: Shintaro Sakoda <[email protected]> --------- Signed-off-by: anhnv3991 <[email protected]> Signed-off-by: Shintaro Sakoda <[email protected]> Co-authored-by: anhnv3991 <[email protected]> Co-authored-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> * fix: add tp to regression test (#44) * Added tp_score to regression_test Signed-off-by: Shintaro Sakoda <[email protected]> * Updated result.csv by result of GitHub Actions Signed-off-by: Shintaro Sakoda <[email protected]> * Updated by the latest result Signed-off-by: Shintaro Sakoda <[email protected]> --------- Signed-off-by: Shintaro Sakoda <[email protected]> * fix: add a param use line search (#46) * Added a parameter "use_line_search" Signed-off-by: Shintaro Sakoda <[email protected]> * Added default value use_line_search=false Signed-off-by: Shintaro Sakoda <[email protected]> --------- Signed-off-by: Shintaro Sakoda <[email protected]> * Refactoring multigrid_ndt_omp Signed-off-by: anhnv3991 <[email protected]> * Refactored multigrid_ndt_omp Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Removed line breaks Signed-off-by: Shintaro Sakoda <[email protected]> * For reference, this should be discared later Signed-off-by: anhnv3991 <[email protected]> * Testing Signed-off-by: anhnv3991 <[email protected]> * Clean up a bit Signed-off-by: anhnv3991 <[email protected]> * Clean up a bit Signed-off-by: anhnv3991 <[email protected]> * Debugging Signed-off-by: anhnv3991 <[email protected]> * Fixed a bug Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Fixed build error Signed-off-by: anhnv3991 <[email protected]> * Updated result.csv Signed-off-by: Shintaro SAKODA <[email protected]> --------- Signed-off-by: anhnv3991 <[email protected]> Signed-off-by: Shintaro Sakoda <[email protected]> Signed-off-by: Shintaro SAKODA <[email protected]> Co-authored-by: anhnv3991 <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> Co-authored-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> * Coding Signed-off-by: anhnv3991 <[email protected]> * Rearrange the code to avoid duplicated processing Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Added multithread voxel grid build Signed-off-by: anhnv3991 <[email protected]> * Fixed build error Signed-off-by: anhnv3991 <[email protected]> * Fixed build errors Signed-off-by: anhnv3991 <[email protected]> * Applied clang Signed-off-by: anhnv3991 <[email protected]> * Added line break at the end of .h and .hpp files Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix * Removed unused include and comments Signed-off-by: anhnv3991 <[email protected]> * Removed unused include and fixed a segmentation fault when sync in setThreadNum Signed-off-by: anhnv3991 <[email protected]> * Fix build error when compare int with size_t Signed-off-by: anhnv3991 <[email protected]> * Fix build error when compare int with size_t Signed-off-by: anhnv3991 <[email protected]> * style(pre-commit): autofix --------- Signed-off-by: anhnv3991 <[email protected]> Signed-off-by: Shintaro Sakoda <[email protected]> Signed-off-by: Shintaro SAKODA <[email protected]> Co-authored-by: anhnv3991 <[email protected]> Co-authored-by: SakodaShintaro <[email protected]> Co-authored-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]>

anhnv3991 and others added 3 commits March 7, 2024 16:40

Refactoring multigrid_ndt_omp

ef7fd7a

Signed-off-by: anhnv3991 <[email protected]>

Refactored multigrid_ndt_omp

d602c54

Signed-off-by: anhnv3991 <[email protected]>

style(pre-commit): autofix

3fed67d

anhnv3991 changed the base branch from master to tier4/main March 7, 2024 14:18

anhnv3991 requested review from SakodaShintaro and YamatoAndo March 7, 2024 14:18

Merge branch 'tier4/main' into refactor/multigrid_ndt_omp

b5512b6

Merge branch 'tier4/main' into refactor/multigrid_ndt_omp

fce6546

SakodaShintaro mentioned this pull request Mar 13, 2024

refactor: multigrid ndt omp pure refactoring #40

Merged

SakodaShintaro added 3 commits March 13, 2024 15:48

Merge branch 'tier4/main' into refactor/multigrid_ndt_omp

4a615a2

Fixed to build

fed085e

Signed-off-by: Shintaro Sakoda <[email protected]>

Removed line breaks

01ae242

Signed-off-by: Shintaro Sakoda <[email protected]>

SakodaShintaro reviewed Mar 13, 2024

View reviewed changes

anhnv3991 and others added 5 commits March 13, 2024 17:17

For reference, this should be discared later

81ab0e5

Signed-off-by: anhnv3991 <[email protected]>

Testing

e75fa6b

Signed-off-by: anhnv3991 <[email protected]>

Clean up a bit

730dd02

Signed-off-by: anhnv3991 <[email protected]>

Clean up a bit

8cc59f2

Signed-off-by: anhnv3991 <[email protected]>

style(pre-commit): autofix

275081c

anhnv3991 requested a review from SakodaShintaro March 14, 2024 00:47

SakodaShintaro and others added 9 commits March 18, 2024 08:42

refactor: organized ndt params (#43)

7225f0e

* Organized ndt params Signed-off-by: Shintaro Sakoda <[email protected]> * style(pre-commit): autofix --------- Signed-off-by: Shintaro Sakoda <[email protected]> Co-authored-by: SakodaShintaro <[email protected]>

fix: add a param use line search (#46)

7850748

* Added a parameter "use_line_search" Signed-off-by: Shintaro Sakoda <[email protected]> * Added default value use_line_search=false Signed-off-by: Shintaro Sakoda <[email protected]> --------- Signed-off-by: Shintaro Sakoda <[email protected]>

Refactoring multigrid_ndt_omp

3993a44

Signed-off-by: anhnv3991 <[email protected]>

Refactored multigrid_ndt_omp

84507ef

Signed-off-by: anhnv3991 <[email protected]>

style(pre-commit): autofix

571f765

Removed line breaks

97996c2

Signed-off-by: Shintaro Sakoda <[email protected]>

For reference, this should be discared later

f24e008

Signed-off-by: anhnv3991 <[email protected]>

anhnv3991 and others added 9 commits March 18, 2024 08:53

Testing

9d5c044

Signed-off-by: anhnv3991 <[email protected]>

Clean up a bit

e3ec8e7

Signed-off-by: anhnv3991 <[email protected]>

Clean up a bit

519237e

Signed-off-by: anhnv3991 <[email protected]>

Debugging

8bb39af

Signed-off-by: anhnv3991 <[email protected]>

Fixed a bug

daa3591

Signed-off-by: anhnv3991 <[email protected]>

Merge branch 'tier4/main' into refactor/multigrid_ndt_omp

b476f10

style(pre-commit): autofix

f0acc00

Fixed build error

30693d0

Signed-off-by: anhnv3991 <[email protected]>

Updated result.csv

07fa763

Signed-off-by: Shintaro SAKODA <[email protected]>

SakodaShintaro approved these changes Mar 19, 2024

View reviewed changes

SakodaShintaro merged commit 9e4544c into tier4/main Mar 19, 2024
5 checks passed

SakodaShintaro deleted the refactor/multigrid_ndt_omp branch March 19, 2024 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor multigrid_ndt_omp #32

Refactor multigrid_ndt_omp #32

anhnv3991 commented Mar 7, 2024

SakodaShintaro commented Mar 11, 2024

anhnv3991 commented Mar 11, 2024

SakodaShintaro left a comment

SakodaShintaro Mar 13, 2024

anhnv3991 Mar 13, 2024

SakodaShintaro Mar 14, 2024 •

edited

Loading

SakodaShintaro left a comment

anhnv3991 commented Mar 19, 2024

Refactor multigrid_ndt_omp #32

Refactor multigrid_ndt_omp #32

Conversation

anhnv3991 commented Mar 7, 2024

SakodaShintaro commented Mar 11, 2024

anhnv3991 commented Mar 11, 2024

SakodaShintaro left a comment

Choose a reason for hiding this comment

SakodaShintaro Mar 13, 2024

Choose a reason for hiding this comment

anhnv3991 Mar 13, 2024

Choose a reason for hiding this comment

SakodaShintaro Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

SakodaShintaro left a comment

Choose a reason for hiding this comment

anhnv3991 commented Mar 19, 2024

SakodaShintaro Mar 14, 2024 •

edited

Loading