Skip to content
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.

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

Add Lie bracket #300

Merged
merged 2 commits into from
Jul 26, 2024
Merged

Add Lie bracket #300

merged 2 commits into from
Jul 26, 2024

Conversation

artivis
Copy link
Owner

@artivis artivis commented Jul 24, 2024

Add the Lie bracket in vector form ([x,y]^v = adj(x) . y).

It is implemented as both :

  • x.bracket(y)
  • Tangent::Bracket(x, y)

with the later being a personal favorite in terms of notation.

The actual bracket in the Lie algebra can then be computed as Tangent::Bracket(x, y).hat().

Note: the vee operator will be implemented in a subsequent pr.

Closes #267 .

Signed-off-by: artivis <deray.jeremie@gmail.com>
@artivis artivis requested a review from joansola July 24, 2024 05:08
@artivis artivis self-assigned this Jul 24, 2024
@artivis artivis added the enhancement New feature or request label Jul 24, 2024
@artivis
Copy link
Owner Author

artivis commented Jul 24, 2024

@joansola Your opinion about the api and more importantly about the fact that it is in vector form? Calling it 'bracket' isn't too much of a stretch since it is actually 'bracket^vee'?

Doing so has the advantage that it doesn't requires the vee operator (which will come in a subsequent pr).

@artivis artivis mentioned this pull request Jul 24, 2024
@nielsvd
Copy link

nielsvd commented Jul 24, 2024

This is great, looking forward to seeing this merged. One question and one comment from my side:

  • Are you planning to add specializations of struct BracketEvaluatorImpl in this PR already? Or is this planned future work?
  • You may consider adding unit tests for the Lie bracket for specific Lie algebras, e.g., for SO3: [V, W] == V.coeffs().cross(W.coeffs()), for Rn and SO2: [V, W] == 0, and perhaps more.

@joansola
Copy link
Collaborator

@joansola Your opinion about the api and more importantly about the fact that it is in vector form? Calling it 'bracket' isn't too much of a stretch since it is actually 'bracket^vee'?

Doing so has the advantage that it doesn't requires the vee operator (which will come in a subsequent pr).

From a mathematical standpoint, yes, our vector representation of the Lie algebra is indeed a Lie algebra, and it has a bracket operator, which is this one, and they are both equivalent, because both spaces are isomorphic.

From a code standpoint. What´s the best way to document this so that a foreign can FIND this documentation trivially? Like, you are on vscode and you hover your mouse over. Then the doc appears and you can read this note just there. Because this is the only point of concern, it does not require a serious documentation, just a note. But this note needs to be findable very easily.

I´d say normally document it in the .h file as any other doxygen documentation. At least this. But being all templated, would the IDEs, by hovering over the function calls, display such doc? I dont know.

@artivis
Copy link
Owner Author

artivis commented Jul 24, 2024

@joansola Regarding documentation, hopefully the doxygen+api is clear enough and should do the job.

@joansola
Copy link
Collaborator

L366   * @return @return LieAlg The Lie bracket [a,b] in vector form.

OK sounds good but here you have two @return statements and say that the return type is LieAlg :-D

@artivis
Copy link
Owner Author

artivis commented Jul 24, 2024

@joansola Fixed the doc.

@artivis
Copy link
Owner Author

artivis commented Jul 24, 2024

@nielsvd Thanks for the feedback.
I've implemented the specialisation for Rn 👍 . However I'll leave the group specific tests as futur work ;)

Copy link
Collaborator

@joansola joansola left a comment

Choose a reason for hiding this comment

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

All seems good to me!!
:-)

@artivis artivis merged commit cd16821 into devel Jul 26, 2024
39 checks passed
@artivis artivis deleted the feat/bracket branch July 26, 2024 12:23
@artivis artivis mentioned this pull request Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lie Bracket operation
3 participants