Vector angles - #1529
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1529 +/- ##
==========================================
- Coverage 62.38% 62.38% -0.01%
==========================================
Files 208 208
Lines 22486 22487 +1
==========================================
Hits 14029 14029
- Misses 8457 8458 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates COMPAS’ vector angle utilities to treat undefined inputs as errors (instead of silently returning 0 / other sentinel values), aligning behavior with the discussion in #1528 and the follow-up context from #1462.
Changes:
angle_vectorsnow raisesValueErrorwhen either input vector has zero length.angle_vectors_projectednow raisesValueErrorwhen an input vector is parallel to the projection normal.- Adds/updates tests and documents the breaking behavior change in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/compas/geometry/test_core.py |
Adds coverage for angle_vectors zero-vector inputs and angle_vectors_projected invalid projection cases. |
src/compas/geometry/_core/angles.py |
Changes core angle helpers to raise ValueError for undefined inputs (zero-length / parallel-to-normal). |
CHANGELOG.md |
Records the breaking behavior changes for both angle functions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| u_cross = cross_vectors(u, normal) | ||
| v_cross = cross_vectors(v, normal) | ||
|
|
||
| if TOL.is_allclose(u_cross, [0.0, 0.0, 0.0]) or TOL.is_allclose(v_cross, [0.0, 0.0, 0.0]): | ||
| raise ValueError("Cannot compute angle between vectors projected onto a plane defined by the normal vector. One of the vectors is parallel to the normal vector.") |
| L = length_vector(u) * length_vector(v) | ||
| if TOL.is_zero(L, tol): | ||
| return 0 | ||
| raise ValueError("Cannot compute the angle between one or more zero-length vectors.") |
There was a problem hiding this comment.
perhaps the length check should be done on the individual vectors. otherwise two small vectors will also trigger the error, even when both vectors individually would qualify for a valid angle calculation
There was a problem hiding this comment.
something like
a = length_vector(u)
b = length_vector(v)
if TOL.is_zero(a, tol) or TOL.is_zero(b, tol):
raise ValueError(...)
cosine = dot_vectors(u, v) / (a * b)There was a problem hiding this comment.
btw, this is more or less what you do in the projected version
This is a follow up to #1462
closes #1528
angle_vectorsnow throws aValueErrorwhen one (or both) of the input vectors are zero vectors.angle_vectors_projected#1462While there's no API change, this could be considered a breaking behavior change so flagged as such.
What type of change is this?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas.datastructures.Mesh.