Skip to content

Make Line3 relation tests invariant to Plücker scale - #232

Open
Doribelove wants to merge 2 commits into
rai-opensource:masterfrom
Doribelove:codex/line3-scale-invariant
Open

Doribelove wants to merge 2 commits into
rai-opensource:masterfrom
Doribelove:codex/line3-scale-invariant

Conversation

@Doribelove

Copy link
Copy Markdown

Summary

Line3.isparallel() compared the raw cross product of Plücker direction vectors to an absolute tolerance. Scaling either representation therefore changed whether perpendicular lines were reported as parallel. The same classification feeds |, isintersecting() / ^, and commonperp().

  • Compare unit directions in isparallel() so the threshold represents angular separation.
  • Normalize both Plücker moments in isintersecting() so its reciprocal-product tolerance is independent of coordinate scale.
  • Build commonperp() from the nearest point and the cross product of unit directions. The previous expression could also produce invalid Plücker coordinates for nonorthogonal skew lines at unit scale.
  • Add regressions for small, large, and sign-reversed scalings, a near-parallel tolerance, crossing and skew lines, and the nearest-point geometry.

For example, lines through (0, 0, 0) along (1e-8, 0, 0) and through (0, 0, 1) along (0, 1e-8, 0) are perpendicular; before this change isparallel() returned True.

This branch is based on current master and does not contain the separate distance() fix in #224. I checked closest_to_line() on the included scaled nonparallel cases; it needs no change for this issue.

Validation

  • New regression tests failed against the original implementation, then passed with the change.
  • pytest -q --disable-warnings: 350 passed, 3 skipped (Python 3.10).
  • Black and git diff --check: passed.
  • Additional local geometric sampling across valid scaled line pairs agreed with the analytical common-perpendicular location to floating-point precision.

Fixes #231.

Implementation and tests were prepared and validated with OpenAI Codex assistance.

Normalize line directions and moments for parallel and intersection checks. Construct common perpendiculars from the nearest point and unit direction cross product, with regression coverage across Plucker scalings.

Assisted-by: OpenAI Codex (GPT-6 Sol)
Signed-off-by: 李永祺 <doribelove@gmail.com>
@petercorke

Copy link
Copy Markdown
Collaborator

Thank you for the quick follow-up, and for closing out #231 so thoroughly!

I independently verified this rather than just re-running your own tests: random-sampled ~10,000 scaled line pairs each for isparallel() and isintersecting() (scale factors from 1e-8 to 1e8, including sign flips) and confirmed the classification never changes with scale. For commonperp() I checked it geometrically rather than via distance() (since this branch doesn't include #224's fix) — for ~2,600 random scaled skew-line pairs, the returned perpendicular line passes through the correct nearest point, is perpendicular to both inputs, and lands exactly on the second line when stepped by the true separation distance. Zero failures across all of that.

Also confirmed this merges cleanly against #224 with no conflicts, and both sets of tests pass together.

Approving — nice work, thanks again.

Keep the near-parallel tolerance test focused on direction vectors by constructing both lines through the origin. This avoids an unrelated Plucker orthogonality check failure on NumPy 2.

Assisted-by: OpenAI Codex (GPT-6 Sol)
Signed-off-by: 李永祺 <doribelove@gmail.com>
@Doribelove

Copy link
Copy Markdown
Author

Thank you for independently validating the geometry and approving the PR.

I found one test-fixture failure in the previous head's codecov job: the large, offset near-parallel line was rejected by PointDir's Plücker orthogonality check due to floating-point cancellation before isparallel() ran. Follow-up 149653a changes only that fixture to pass through the origin; its direction and the angular-tolerance assertion are unchanged.

On the new head, the full suite passes locally with Python 3.10.12 / NumPy 2.2.6 (350 passed, 3 skipped) and Python 3.12.14 / NumPy 2.5.3 under pytest-cov (350 passed, 3 skipped). Black and git diff --check also pass. The new fork-PR CI run is currently action_required, so I am not claiming a remote result for this commit yet. The previous Sphinx failure is the known fork publishing issue tracked separately by #222.

This follow-up was prepared and validated with OpenAI Codex assistance on behalf of @Doribelove.

This branch has not been deployed

No deployments
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.

Line3.isparallel()/__or__ still scale-dependent, like the distance() bug fixed in #224

2 participants