Replaced faer SVD with kornia_linalg::svd3 in ICP crate #337
Conversation
|
Could you please review this? I noticed that a PR has already been created for this, but it isn't solving the issue. Therefore, I created this PR, which correctly addresses the main issue |
crates/kornia-icp/src/ops.rs
Outdated
| let p_dst = faer::col![p_in_dst[0], p_in_dst[1], p_in_dst[2]] - &dst_centroid; | ||
| hh += p_src * p_dst.transpose(); | ||
| let p_src = Vec3::new( | ||
| (p_in_src[0] - src_centroid.x) as f32, |
There was a problem hiding this comment.
This can be done directly with glam semantics as before
crates/kornia-icp/src/ops.rs
Outdated
| } | ||
|
|
||
| // compute translation vector t = C_dst - R * C_src | ||
| let t = dst_centroid - &rr * src_centroid; | ||
| let src_centroid_f32 = Vec3::new(src_centroid.x as f32, src_centroid.y as f32, src_centroid.z as f32); |
There was a problem hiding this comment.
Why the casting ? Precision should be kept
crates/kornia-icp/src/ops.rs
Outdated
|
|
||
| for (p1, p2) in points1.iter().zip(points2.iter()) { | ||
| centroid1 += faer::col![p1[0], p1[1], p1[2]]; | ||
| centroid2 += faer::col![p2[0], p2[1], p2[2]]; | ||
| centroid1 += Vec3::new(p1[0] as f32, p1[1] as f32, p1[2] as f32); |
| @@ -65,17 +81,17 @@ pub(crate) fn fit_transformation( | |||
| pub(crate) fn compute_centroids( | |||
There was a problem hiding this comment.
To make it easy, inputs should be also ported
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the previous faer SVD implementation with the optimized kornia_linalg::svd3 function in the ICP crate, while converting matrix types from faer to glam.
- Removed the faer dependency and added kornia-linalg in Cargo.toml
- Updated ops.rs to use glam’s Mat3 and Vec3 along with kornia_linalg’s svd3
- Revised tests to use assert_relative_eq for floating-point comparisons
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/kornia-icp/src/ops.rs | Updated matrix and vector operations, SVD computation, and conversion logic |
| crates/kornia-icp/Cargo.toml | Updated dependencies by removing faer and adding kornia-linalg |
Ya-shh
force-pushed
the
update-icp-with-svd3
branch
from
002f1bc to
24ecbd8
Compare
|
|
@edgarriba Thanks for pointing out these issues. Now the tests are passing, and I have run them locally |
|
@edgarriba The actual switch to kornia_linalg::svd3 will require addressing precision issues since kornia_linalg works with f32 (glam::Mat3) while the current code uses f64 (faer::Mat). |
This PR updates the kornia-icp crate to use the optimized kornia_linalg::svd3 function instead of faer's SVD implementation.
Changes made:
These changes should make the ICP algorithms more efficient for SVD operations on 3×3 matrices while maintaining the same mathematical accuracy.