Quality: Zero-length vector normalization can produce NaN basis vectors#775
Conversation
`get_unit_vector` divides by `np.linalg.norm(vector)` without checking for zero (or near-zero) norm. If two ChArUco points coincide or data is degenerate/noisy, this returns `inf`/`nan` and silently corrupts the computed coordinate basis (`x_hat`, `y_hat`, `z_hat`), leading to invalid calibration output downstream. Affected files: charuco_groundplane_utils.py Signed-off-by: Nguyen Huy Hoang <181364121+huyhoang171106@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves robustness of ChArUco ground-plane calibration by preventing zero/invalid vector normalization from silently producing NaN basis vectors, which can corrupt downstream calibration outputs.
Changes:
- Add a domain-specific
CharucoNormalizationErrorfor normalization failures. - Guard
get_unit_vectoragainst zero/near-zero and non-finite norms and raise instead of returning invalid values.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Raised when the velocity of the ChArUco corners is too high to be considered stationary.""" | ||
|
|
||
|
|
||
| class CharucoNormalizationError(RuntimeError): |
There was a problem hiding this comment.
CharucoNormalizationError is introduced here but isn’t handled by the main ground-plane path (e.g., AniposeCameraCalibrator.set_charuco_board_as_groundplane currently catches CharucoVisibilityError/CharucoVelocityError only). As a result, degenerate ChArUco geometry will now hard-fail calibration instead of gracefully skipping ground-plane alignment. Consider updating the caller(s) to catch CharucoNormalizationError and return a GroundPlaneSuccess(success=False, error=...) similar to the other ChArUco error cases (or otherwise intentionally document that this error should be fatal).
| class CharucoNormalizationError(RuntimeError): | |
| class CharucoNormalizationError(CharucoVelocityError): |
| if not np.isfinite(norm) or np.isclose(norm, 0.0): | ||
| raise CharucoNormalizationError("Cannot normalize a zero-length or invalid vector.") | ||
| return vector / norm |
There was a problem hiding this comment.
The raised CharucoNormalizationError message is the same for all call sites, but get_unit_vector is used to normalize multiple different vectors (x, y, cross products). When this triggers, it will be hard to tell which vector was invalid and why. Consider including norm (and possibly the vector name or contents) in the error message, or wrapping calls in compute_basis_vectors_of_new_reference to re-raise with context about which basis component failed.
| if not np.isfinite(norm) or np.isclose(norm, 0.0): | ||
| raise CharucoNormalizationError("Cannot normalize a zero-length or invalid vector.") |
There was a problem hiding this comment.
np.isclose(norm, 0.0) uses NumPy defaults (atol=1e-8, rtol=1e-5), which makes the “near-zero” cutoff somewhat implicit and not obviously tied to the scale/units of the ChArUco reconstruction. Consider specifying an explicit tolerance (e.g., atol=... in world units) so the normalization guard is stable and intention-revealing.
Code Quality
Problem
get_unit_vectordivides bynp.linalg.norm(vector)without checking for zero (or near-zero) norm. If two ChArUco points coincide or data is degenerate/noisy, this returnsinf/nanand silently corrupts the computed coordinate basis (x_hat,y_hat,z_hat), leading to invalid calibration output downstream.Severity:
highFile:
freemocap/core_processes/capture_volume_calibration/anipose_camera_calibration/charuco_groundplane_utils.pySolution
Guard against zero/near-zero norm and raise a domain-specific error instead of returning invalid values. Example:
Changes
freemocap/core_processes/capture_volume_calibration/anipose_camera_calibration/charuco_groundplane_utils.py(modified)Testing
Closes #774