Conversation
📝 WalkthroughWalkthroughThe PR introduces automatic Birge-ratio-style balancing of cov-to-cov residual contributions in the Gauss-Newton optimizer. Two new configuration parameters ( ChangesCov-to-Cov Scaling and Prior Balancing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
agents.md (1)
202-202: ⚡ Quick winClarify “long hyphens” with explicit punctuation names.
Line 202 is a bit ambiguous as written. Please name the exact characters (for example, “en dash (–)” / “em dash (—)”) so contributors can apply the rule consistently.
Suggested wording
-- Don't use long hyphens. Use American spelling. +- Avoid en/em dashes (–, —); use the regular hyphen-minus (-). Use American spelling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents.md` at line 202, Replace the ambiguous phrase "long hyphens" with explicit punctuation names and examples: state "en dash (–) and em dash (—)" and instruct using American spelling (e.g., "color" not "colour") so contributors know exactly which characters to avoid; update the sentence containing "long hyphens" to read something like: "Do not use en dash (–) or em dash (—). Use American spelling." and ensure the phrase "long hyphens" is removed or clarified (search for the exact string "long hyphens" to find the location).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mp2p_icp/src/optimal_tf_gauss_newton.cpp`:
- Around line 291-299: Before the iterative solver starts, validate
gnParams.cov2cov_alpha to ensure it's finite and positive (e.g.
std::isfinite(gnParams.cov2cov_alpha) && gnParams.cov2cov_alpha > 0); if the
check fails, log an error and return false (fail fast) so the solver doesn't
proceed with an invalid global scale; add this single guard near the beginning
of the function that contains the Gauss-Newton loop (reference symbols:
cov2cov_alpha, gnParams, chi2_cc / nCov2Cov usage) so cc_scale is never
initialized from a bad value.
---
Nitpick comments:
In `@agents.md`:
- Line 202: Replace the ambiguous phrase "long hyphens" with explicit
punctuation names and examples: state "en dash (–) and em dash (—)" and instruct
using American spelling (e.g., "color" not "colour") so contributors know
exactly which characters to avoid; update the sentence containing "long hyphens"
to read something like: "Do not use en dash (–) or em dash (—). Use American
spelling." and ensure the phrase "long hyphens" is removed or clarified (search
for the exact string "long hyphens" to find the location).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc789ff1-83bd-49be-8e2c-982d82bf9d6a
📒 Files selected for processing (5)
agents.mdmp2p_icp/include/mp2p_icp/optimal_tf_gauss_newton.hmp2p_icp/src/optimal_tf_gauss_newton.cpptests/CMakeLists.txttests/test-mp2p_optimize_cov2cov_with_prior.cpp
Comment on lines
+291
to
+299
| { | ||
| double cc_scale = gnParams.cov2cov_alpha; | ||
| if (gnParams.cov2cov_auto_balance_with_prior && gnParams.prior.has_value() && | ||
| nCov2Cov >= 3) | ||
| { | ||
| const double dof = std::max(1.0, 3.0 * static_cast<double>(nCov2Cov) - 6.0); | ||
| const double kappa = std::max(1.0, chi2_cc / dof); | ||
| cc_scale /= kappa; | ||
| } |
There was a problem hiding this comment.
Validate cov2cov_alpha before using it as a global scale.
A negative or non-finite cov2cov_alpha will make the normal equations invalid and can silently propagate NaNs or an indefinite Hessian while still returning true. Please fail fast on invalid values once, before the iteration loop.
Suggested guard
+#include <cmath>
...
ASSERTMSG_(
gnParams.linearizationPoint.has_value(), "This method requires a linearization point");
+ ASSERTMSG_(
+ std::isfinite(gnParams.cov2cov_alpha) && gnParams.cov2cov_alpha >= 0.0,
+ "`cov2cov_alpha` must be finite and non-negative");
result.optimalPose = gnParams.linearizationPoint.value();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mp2p_icp/src/optimal_tf_gauss_newton.cpp` around lines 291 - 299, Before the
iterative solver starts, validate gnParams.cov2cov_alpha to ensure it's finite
and positive (e.g. std::isfinite(gnParams.cov2cov_alpha) &&
gnParams.cov2cov_alpha > 0); if the check fails, log an error and return false
(fail fast) so the solver doesn't proceed with an invalid global scale; add this
single guard near the beginning of the function that contains the Gauss-Newton
loop (reference symbols: cov2cov_alpha, gnParams, chi2_cc / nCov2Cov usage) so
cc_scale is never initialized from a bad value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Tests