feat: Implement MultiVector.log() principal branch inverse of exp()#132
feat: Implement MultiVector.log() principal branch inverse of exp()#132sunkmechie wants to merge 15 commits into
Conversation
|
Thank you for this great PR, this is definitely something we want to have. Overall I think this is a great PR and already looks very promising. I do however have some comments:
For a rotation you would then set
Looking forward to your reply. |
|
Thank you for the detailed feedback and the kind words. I will refactor the PR to:
I love the For the non-simple test case, we can perhaps use two skew lines? I'll push these updates shortly! |
… improved array handling
|
Thanks again for the detailed review, Martin! I’ve pushed an update addressing the points you raised. Key Changes:
I used >>> from kingdon import Algebra
>>> pga3d = Algebra.fromname('3DPGA')
>>> B = pga3d.multivector(e03=1, e12=1)
>>>
>>> print((B * B).filter()) # scalar + grade-4 part
-1 + 2 e0123
>>> print((B ^ B).filter()) # nonzero grade-4 part
2 e0123
>>> (1 + B).log() # Raises NotImplementedError as expected
Traceback (most recent call last):
...
NotImplementedError: Currently only rotors with a simple bivector part can be logarithmized.I’ve also added tests for the other exception cases and reran the full test suite. To keep Looking forward to your thoughts! |
| e02=np.array([0.5, 0.0, 0.0]), | ||
| e12=np.array([0.0, 0.7, 1.2]), | ||
| ) | ||
| R_arr = alg.multivector( |
There was a problem hiding this comment.
I would put scale outside as a single multiply to prevent mistakes. R_arr = scale * alg.multivector(...)
There was a problem hiding this comment.
Thanks for the feedback, I've refactored to have the scale as a common multiplier for better clarity.
|
|
||
| pga3d = Algebra.fromname('3DPGA') | ||
| with pytest.raises(NotImplementedError, match='simple bivector part'): | ||
| pga3d.multivector(e=1, e03=1, e12=1).log() |
There was a problem hiding this comment.
This has a non-simple bivector part, but it is not a valid rotor, as that should be the exp of bivector and this is not. You need to have a test input of the form
There was a problem hiding this comment.
Great point on the validity of the rotor manifold. I've updated the test case to use a valid screw motion constructed from the product of two skew exponentials (
All the tests are passing locally, looking forward to your review.
|
|
||
| # 3DPGA checks the same logic in a 4D algebra where non-simple bivectors can exist. | ||
| pga3d = Algebra.fromname('3DPGA') | ||
| B_pga3d = pga3d.bivector(e12=0.8) |
There was a problem hiding this comment.
Good point. I'll include in in the tests in next update.
| alg.multivector(e=1).log(arctanh2=lambda y, x: 0) | ||
|
|
||
| with pytest.raises(TypeError, match='complex values are not supported'): | ||
| alg.multivector(e=1 + 0j).log() |
There was a problem hiding this comment.
I actually think complex values should be supported, but I am not sure how to interpret this test. Would this error also be raised if you take the log of a more interesting complex simple rotor? In our paper we have an example in R2,2 where complex numbers are needed in the invariant decomposition of certain rotors, so this is something we absolutely need to support.
https://www.researchgate.net/publication/370750268_Graded_Symmetry_Groups_Plane_and_Simple
There was a problem hiding this comment.
Thanks for pointing this out and for linking the paper.
The current implementation assumes real coefficients (might actually have been a regression, seeing how exp() supported them). I'll revisit this restriction while refactoring and check how it should behave for more general complex simple rotors.
| l = sqrt(ll) | ||
| return self * sinhc(l) + cosh(l) | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
I do not like all these new staticmethods on MultiVector that do not have anything to do with multivector per se. I would propose that you make a new file, log.py and place everything there, and then just add a simple call to the log entry point on MultiVector itself. That way we keep MultiVector as clean as possible.
There was a problem hiding this comment.
That makes sense. I introduced the helpers to keep the main log() implementation close in style to exp(), but I agree it ended up cluttering MultiVector.
I'll move the implementation into a separate log.py module and keep MultiVector.log() as a thin entry point.
| return MultiVector._truthy(value < 0) | ||
|
|
||
| @staticmethod | ||
| def _call_helper(func, *args, **kwargs): |
There was a problem hiding this comment.
If we need a helper like this to call a function then something is going very wrong :P. Your first PR looked man-made but now I am beginning to doubt it, tell Claude to take a chill-pill :P.
There was a problem hiding this comment.
Fair point. I will remove the helper and simplify in the next update.
| return sqrt, arctanh2 | ||
|
|
||
| if isinstance(sq, complex) or isinstance(c_val, complex): | ||
| raise TypeError( |
There was a problem hiding this comment.
See my other comment about complex.
|
I started restructuring the implementation as suggested. For now I moved the structure into log.py and left placeholders while I refactor the logic. I'll fill in the implementation in the next commits. |
|
Thank you for your comprehensive feedback and the time you spent reviewing. Following your detailed review @tBuLi , I refactored the Breakdown of the changes: 1. Complex Rotors &
|
|
Hi @sunkmechie, thanks for the updated PR! I will review it later, but I just wanted to answer your question already: non-simple bivectors/rotors are absolutely on the roadmap for future PR's. |
Closes #131
Summary
This PR adds
MultiVector.log()as the principal inverse ofexp()for normalized simple rotors.It covers the three standard cases for a rotor
r = c + B:B^2 < 0B^2 > 0B^2 = 0What changed
MultiVector.log()inkingdon/multivector.py-1log()usageNotes
log()is implemented for normalized simple rotors over real scalar/array inputs and symbolic expressions.Complex-valued inputs are intentionally not supported in this PR. The current implementation relies on the real-valued split between
B^2 < 0,B^2 > 0, andB^2 = 0, together with realatan2/atanhbehavior. A correct complex implementation would be difficult and is best done in a separate PR.