FIX: georeferencing yaml serialization missed orientation fields

[jlblancoc]

Summary by CodeRabbit

• Tests

  • Added comprehensive test coverage for georeferencing data serialization and deserialization, including round-trip validation, error handling, and edge cases.

• Refactor

  • Internal improvements to georeferencing data serialization implementation for enhanced consistency.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refactors the georeferencing YAML serialization in mp2p_icp by renaming the parameter from gref to geoRef, refactoring FromYAML to use a CPose3D constructor, updating YAML key names to use degree notation (yaw_deg, pitch_deg, roll_deg), and adding a comprehensive test suite covering round-trip serialization scenarios.

Changes

Cohort / File(s)	Summary
Georeferencing Serialization mp2p_icp_map/include/mp2p_icp/metricmap.h, mp2p_icp_map/src/metricmap.cpp	Parameter renamed from gref to geoRef across declarations and implementations. FromYAML refactored to construct CPose3D using FromXYZYawPitchRoll instead of setting components individually. ToYAML updated to use degree-based YAML keys (yaw_deg, pitch_deg, roll_deg) and simplified with local constant reference for pose access.
Testing Infrastructure tests/CMakeLists.txt, tests/test-georef-yaml.cpp	Two new test targets registered (georef-yaml, mp2p_georef_yaml). Comprehensive 601-line test suite added covering nullopt round-trips, non-trivial poses with varying covariance, schema validation, degree-radian fidelity, error paths (missing/wrong type), and text serialization round-trips.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• sm2mm: new feature to directly use georef as input and produce maps (and apply filters!) in ENU frame #46: Modifies the same georeferencing/metric_map YAML serialization infrastructure (Georeferencing struct and its I/O functions).

Poem

🐰 With whiskers twitching bright and keen,
I've hopped through fields of YAML scene,
Where geoRef hops with graceful ease,
And poses serialize with please,
Through tests galore, this code takes flight! 🌿

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: adding missing orientation fields (yaw_deg, pitch_deg, roll_deg) to georeferencing YAML serialization, which aligns with the core changes across the codebase.
Docstring Coverage	✅ Passed	Docstring coverage is 90.48% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/georef-yaml-missing-fields

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/test-georef-yaml.cpp (1)

83-92: Covariance matrix is not symmetric.

The generated covariance matrix uses (r+1)*(c+1)*0.001 which produces a symmetric matrix (since (r+1)*(c+1) == (c+1)*(r+1)), so this is actually correct. However, for covariance matrices to be valid, they must also be positive semi-definite. This particular pattern may not satisfy that property.

For test purposes this likely doesn't matter since you're just verifying round-trip fidelity, but if MRPT ever validates covariance matrices, the tests could fail unexpectedly.

💡 Optional: Use a guaranteed positive-definite covariance

     // Non-zero, non-diagonal covariance
-    g.T_enu_to_map.cov.setZero();
-    for (int r = 0; r < 6; ++r)
-    {
-        for (int c = 0; c < 6; ++c)
-        {
-            // Use a recognisable pattern: (r+1)*(c+1)*0.001, symmetric
-            g.T_enu_to_map.cov(r, c) = static_cast<double>((r + 1) * (c + 1)) * 0.001;
-        }
-    }
+    // Build a positive-definite covariance: C = A * A^T where A has distinct values
+    mrpt::math::CMatrixDouble66 A;
+    for (int r = 0; r < 6; ++r)
+        for (int c = 0; c < 6; ++c)
+            A(r, c) = (r == c) ? (r + 1) * 0.1 : (r + c) * 0.01;
+    g.T_enu_to_map.cov = A * A.transpose();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test-georef-yaml.cpp` around lines 83 - 92, The covariance assigned to
g.T_enu_to_map.cov uses a patterned fill that may not be positive-definite;
replace the current element-wise fill (the loop writing g.T_enu_to_map.cov(r,
c)) with a construction that guarantees positive-definiteness, e.g. build a
small full-rank matrix M and set g.T_enu_to_map.cov = M * M.transpose() or
create a diagonal-dominant symmetric matrix by initializing diagonal entries to
sufficiently large positive values and mirroring off-diagonals so the resulting
matrix is symmetric and positive-definite; update the code around
g.T_enu_to_map.cov.setZero() and the following loop to use one of these
approaches.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/test-georef-yaml.cpp`:
- Around line 83-92: The covariance assigned to g.T_enu_to_map.cov uses a
patterned fill that may not be positive-definite; replace the current
element-wise fill (the loop writing g.T_enu_to_map.cov(r, c)) with a
construction that guarantees positive-definiteness, e.g. build a small full-rank
matrix M and set g.T_enu_to_map.cov = M * M.transpose() or create a
diagonal-dominant symmetric matrix by initializing diagonal entries to
sufficiently large positive values and mirroring off-diagonals so the resulting
matrix is symmetric and positive-definite; update the code around
g.T_enu_to_map.cov.setZero() and the following loop to use one of these
approaches.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4585b4c5-13ac-4961-9007-5b0a921d9221

📥 Commits

Reviewing files that changed from the base of the PR and between b01daeb and f4ba146.

📒 Files selected for processing (4)

• mp2p_icp_map/include/mp2p_icp/metricmap.h
• mp2p_icp_map/src/metricmap.cpp
• tests/CMakeLists.txt
• tests/test-georef-yaml.cpp

[codecov]

Codecov Report

❌ Patch coverage is 96.01449% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.60%. Comparing base (b01daeb) to head (f4ba146).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
tests/test-georef-yaml.cpp	95.68%	11 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #47      +/-   ##
===========================================
+ Coverage    78.17%   78.60%   +0.43%     
===========================================
  Files          189      190       +1     
  Lines        10305    10564     +259     
  Branches       968      979      +11     
===========================================
+ Hits          8056     8304     +248     
- Misses        2249     2260      +11     

Files with missing lines	Coverage Δ
mp2p_icp_map/include/mp2p_icp/metricmap.h	86.66% <ø> (ø)
mp2p_icp_map/src/metricmap.cpp	42.72% <100.00%> (+0.55%)	⬆️
tests/test-georef-yaml.cpp	95.68% <95.68%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.