catch for vector parallel to normal in angle_vectors_projected

[obucklin]

This fix catches the case where one or more of the vectors to be measured is parallel to the projection normal. This was returning an angle of 0, and it now returns None

What type of change is this?

• Bug fix in a backwards-compatible manner.
• New feature in a backwards-compatible manner.
• Breaking change: bug fix or new feature that involve incompatible API changes.
• Other (e.g. doc update, configuration, etc)

Checklist

Put an x in 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.

• I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
• I ran all tests on my computer and it's all green (i.e. invoke test).
• I ran lint on my computer and there are no errors (i.e. invoke lint).
• I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
• I have added tests that prove my fix is effective or that my feature works.
• I have added necessary documentation (if appropriate)

[obucklin]

@chenkasirer @gonzalocasas could one of you please take a look at this? thanks!!

[chenkasirer]

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.

[chenkasirer]

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

[tomvanmele]

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" :)

[tomvanmele]

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

[papachap]

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?

[chenkasirer]

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

[obucklin]

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.

[tomvanmele]

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?

[obucklin]

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.

[chenkasirer]

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?