search

tulip-control / polytope

Geometric operations on polytopes of any dimension
https://pypi.org/project/polytope
Other
73 stars 19 forks source link

Added assert to check non-zero, non-colinear u,v #91

Closed drscotthawley closed 4 months ago

drscotthawley commented 4 months ago

How well does this satisfy your TODO?

slivingston commented 4 months ago

If indeed the algorithm allows the vectors to be zero or parallel, then this function should not raise an exception. However, the resulting matrix may have some numerical error (i.e., not exactly be the identity). Likely the user should instead call np.eye() to create a rotation matrix or not "rotate" at all, so I recommend that a warning is logged.

drscotthawley commented 4 months ago

Oooo excellent points, thank you. Failing an assert is overly harsh, in retrospect. It would make sense that if u & v are (anti-)parallel, it could return the identity matrix without executing the rest of the function. UserWarning in the case of a zero vector seems good.
I'll work on those changes and resubmit. And even then, no worries if my changes aren't your cup tea. I just saw something I could add quickly.

drscotthawley commented 4 months ago

After doing some more checking, it seems the algorithm already robustly returns an identify matrix for the cases of zero or (anti-)parallel vectors. Thus one can, if one wants, add a check such as in the new revision in order to maybe save a bit of time (and maybe get a little extra numerical precision) in the (rare) cases when this applies. It is, however, unnecessary. (And the checking will add a bit of time to all other cases.) ...So one could simply remove the "TODO" comment and leave the original code as-is. Feel free to close with or without accepting this PR. Thanks for posting your code!