Skip to content

fix: infer default normals in vector angle helpers - #906

Merged
iory merged 1 commit into
iory:mainfrom
HaokaiDing:fix/default-vector-angle-normal
Sep 16, 2026
Merged

iory merged 1 commit into
iory:mainfrom
HaokaiDing:fix/default-vector-angle-normal

Conversation

@HaokaiDing

Copy link
Copy Markdown
Contributor

Fixes #905.

Both oriented-angle helpers accept normal_vector=None, but their default branches call the parameter itself, raising TypeError: 'NoneType' object is not callable.

Use the existing normalize_vector helper on the cross product, and document how the default normal is inferred. For [1, 0, 0] and [0, 1, 0], the functions now return pi/2 counter-clockwise and 3*pi/2 clockwise. Explicit normals retain their existing behavior.

Validation:

  • All 10 new regression cases fail before the fix.
  • .venv/bin/python -m pytest tests/skrobot_tests/coordinates_tests -q: 147 passed.
  • Cases cover both helpers, omitted and explicit-None normals, non-unit vectors, a 3D plane, and parallel/antiparallel vectors.
  • Non-unit input [2, 0, 0] / [3, 3, 0] verifies that the cross product is normalized, rather than merely avoiding the exception.
  • Ruff, Flake8 and git diff --check pass on the changed files.

Tested with Python 3.13.15, NumPy 2.5.3 and SciPy 1.18.1 on macOS. The robot, renderer and simulator test suites were not run for this coordinates-only fix.

Copilot AI lite review requested due to automatic review settings September 15, 2026 16:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@iory

iory commented Sep 16, 2026

Copy link
Copy Markdown
Owner

Thanks for the report and the patch. I reviewed this and reproduced your validation locally on a checkout of the PR head (81d675c, based on the current main 68ef23e, so no rebase is needed). Summary: looks good to me.

Verification

  • Reverting only skrobot/coordinates/math.py to 68ef23e and running the new cases: 10 failed. With the fix, pytest tests/skrobot_tests/coordinates_tests -q gives 147 passed (Python 3.13, NumPy 2.5, SciPy 1.18, macOS) — matches your numbers.
  • The normalization is doing real work, not just avoiding the exception. For [2, 0, 0] / [3, 3, 0]: the fix returns 0.785398 (pi/4), while an unnormalized np.cross normal returns 1.405648.
  • The reason is worth stating explicitly: with a unit normal, det = |v1||v2| sin(theta) and dot = |v1||v2| cos(theta) share the same scale, so arctan2 becomes scale invariant. I checked this over 2000 random vector pairs — ccw(a, b) == ccw(7.3 * a, 0.11 * b) within 1e-12, and ccw + cw == 2 * pi (mod 2 * pi) throughout.
  • The degenerate cross product is safe because normalize_vector returns the zero vector for zero norm instead of dividing, so parallel/antiparallel inputs give 0 and pi as your tests assert.
  • No behavior change for callers passing an explicit normal — that path is untouched. Grepping the repo, nothing outside math.py and the tests calls either helper, so the broken branch was unreachable from library code and this is a public-API-only fix.
  • flake8 (repo .flake8) and ruff check are clean on both changed files.

One optional suggestion

When the normal is inferred from v1 x v2, the counter-clockwise result is by construction always within [0, pi] and never in (pi, 2 * pi) (max 3.0690 over the 2000 random pairs above); symmetrically the clockwise result is always >= pi. Both docstrings still say Return values in [0 radian, 2 * np.pi radian], which stays true but is looser than what the default branch can actually produce. A single sentence noting that would make the new default easier to reason about. Not a blocker.

(Also noting for context that the Copilot review on this PR never ran — it reported a quota limit.)

@iory iory left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving: verified locally as described above. The 10 new cases fail on the pre-fix math.py and the full tests/skrobot_tests/coordinates_tests suite passes (147) with the fix, lint is clean, and the explicit-normal path is unchanged. Will merge once CI is green.

@iory
iory merged commit b21e769 into iory:main Sep 16, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Vector angle helpers fail when normal_vector is omitted

3 participants