Skip to content

catch for vector parallel to normal in angle_vectors_projected #1462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

catch for vector parallel to normal in angle_vectors_projected #1462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2c0e481
Merge branch 'main' of https://github.com/obucklin/compas
obucklin Apr 7, 2025
30f7d0e
Merge branch 'main' of https://github.com/compas-dev/compas
obucklin May 15, 2025
5b10b45
added catch for vector parallel to normal
obucklin May 15, 2025
1af2b03
Changelog
obucklin May 15, 2025
b75d2d7
changed to raise ValueEror
obucklin Jun 11, 2025
6970beb
Merge branch 'main' of https://github.com/compas-dev/compas
obucklin Jun 11, 2025
0597af4
Merge branch 'main' into fix_angle_vectors_projected
obucklin Jun 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* Fixed a bug when printing an empty `Tree`.
* Fixed a bug in `Group` for IronPython where the decoding declaration was missing.
* Fixed a bug where a `Group` without name could not be added to the scene.
* Fixed a bug where `angle_vectors_projected` returned a 0 when an input vector was parallel to projection normal. Now returns `None`.

### Removed

Expand Down
3 changes: 3 additions & 0 deletions src/compas/geometry/_core/angles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ def angle_vectors_projected(u, v, normal, deg=False, tol=None):
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]):
Copy link
Member

Choose a reason for hiding this comment

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

mmh just a thought here, I'm not sure this is a bug in this particular function. like if you pass u or v such that one or both of them are parallel to normal the angle between u and v projected on the plane IS in-fact 0, right?

from an API perspective (well mine ;) we'd be forcing angle_vectors_projected here to communicate to us something it's not necessarily concerned with.

Copy link
Member

Choose a reason for hiding this comment

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

of course if projecting v onto plane with normal v is mathematically undefined then I'd perhaps raise ValueError here rather than return None, for explicity's sake

Copy link
Member

Choose a reason for hiding this comment

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

i agree with @chenkasirer

the angle between two vectors can't be None.
the function should either return a float or throw an error.

and it should only throw an error if the angle cannot be computed form the input data, not because the result value is "inconvenient" :)

Copy link
Member

Choose a reason for hiding this comment

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

if the project onto the plane of one of the two vectors is a vector of zero length, then the angle wrt that plane is indeed 0 in my opinion

Copy link
Contributor

Choose a reason for hiding this comment

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

returning 0 when one of the vectors is zero-length is more of a convention, right? since in strict math terms the angle is undefined (division by zero)

this is more an argument for throwing an error, unless it's a common convention that i am just not familiar with?

Copy link
Member

Choose a reason for hiding this comment

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

I'm no mathematician but according to the discussion here the angle between a vector and a zero vector is indeed undefined, as @papachap said. unfortunately no linear algebra books around me but if this is correct then i guess it supports raising an error, which should maybe done here?:

compas/src/compas/geometry/_core/angles.py

Lines 47 to 49 in ba669cb

L = length_vector(u) * length_vector(v)
if TOL.is_zero(L, tol):
return 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me, returning 0 is clearrly incorrect, as that is a valid float indicating parallel vectors. From the comments, it seems raising a ValueError seems to be the way to go.

Copy link
Member

Choose a reason for hiding this comment

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

could you perhaps post a snippet in which this specific case would be triggered, and how it is ideally handled? would you indeed expect the code to stop, and add an error catching block to handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the current usage:

inclination = angle_vectors_projected(zzaxis, front_plane.normal, yyaxis)
if inclination is None:
    inclination = angle_vectors_signed(zzaxis, ref_side.xaxis, ref_side.normal, deg=True)

I would go ahead and change this to:

if is_parallel_vector_vector(zzaxis, front_plane.normal) or is_parallel_vector_vector(zzaxis, yyaxis):
    inclination = angle_vectors_signed(zzaxis, ref_side.xaxis, ref_side.normal, deg=True)
else:
    inclination = angle_vectors_projected(zzaxis, front_plane.normal, yyaxis)

I don't expect any particular behavior, other than it should not return 0. I could imagine that being mathematically undefined, as stated by @papachap, could equate to returning a None. However it seems that raising an error is preferred. I will go ahead and make that change for y'all's consideration.

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.")
Copy link
Member

Choose a reason for hiding this comment

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

I think the right place to put this is

compas/src/compas/geometry/_core/angles.py

Lines 47 to 49 in ba669cb

L = length_vector(u) * length_vector(v)
if TOL.is_zero(L, tol):
return 0

however, that's potentially a breaking change. since angle_vectors_projected is relatively new and specialized, perhaps we keep it here for now. @tomvanmele any objections to merging this?


return angle_vectors_signed(u_cross, v_cross, normal, deg, tol)


Expand Down
Loading