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

Match the Manifolds.jl notation in doc #839

Open
Affie opened this issue Apr 6, 2022 · 2 comments
Open

Match the Manifolds.jl notation in doc #839

Affie opened this issue Apr 6, 2022 · 2 comments

Comments

@Affie
Copy link
Member

Affie commented Apr 6, 2022

Nice!

I have two short comments after a short first look: In the intro you introduce Lie groups – at the end you link to JuliaManifolds/Manifolds.jl, but you could actually link to the documentation about GroupManifolds? https://juliamanifolds.github.io/Manifolds.jl/latest/manifolds/group.html

Second, more e technical remark: We redesigned ManifoldsBase to have a neat trait system, so AbstractGroupManifold will vanish in the next (breaking) release of Manifolds.jl (based on the already released ManifoldsBase.jl 0.13.0 with its own documentation now) and there will just be a IsGroupManifold trait.

Oh I just saw two further things down the road:

  • You write $T_M(p)$ for the tangent space, we actually write $T_p\mathcal M$ (manifolds are calligraphic). This is just a remark, if you have a different notation that is fine of course, if you just use this notation in this one place, maybe it might be helpful to the reader to use only one notation.
  • you write $vee$. and $Euclidean$ and such in Math mode. To me that reads as variables multiplied (so it would acutally be $e^2v$), since they are in italics, the function would be $\mathrm{vee}$ (but that's also just me, a math and notation nerd).

Originally posted by @kellertuer in JuliaManifolds/Manifolds.jl#355 (comment)

@dehann
Copy link
Member

dehann commented Apr 6, 2022

Hi @kellertuer, great, thanks for the notes and pointers on ManifoldsBase.jl v0.13.0. Will do the fixes.

I caught the wave to hold back with the updates until Manifolds.jl is tagged but have not yet been able to dig through all the code to learn what the changes were. I saw some of those PRs were pretty large.

So the few comments here help us a lot, thanks!

Oh yeah, I strongly second the notation nerd comment, I'm biased to following the notation you have in JuliaManifolds. For us working the navigation problem, the number one issue is ALWAYS figuring out what all conventions are, as used by different groups. So notation is super super important and my sense is there should be zero friction when looking at the factor graph solver here or the JuliaManifolds ecosystem. If we stray but a little, we are going to get these impossible error stacks and funky numerics and then trying to debug will be a nightmare.

@kellertuer
Copy link

For the notation – sure I am not saying you must change it, just if you could mention that the notation is different in Manifolds.jl or something.

An also for the update – suer, we still need a few more iterations to finish the Manifolds.jl release – Manopt is already done for example – but Manifolds.jl has just a few small errors to fix left.

@dehann dehann added this to the v0.0.x milestone Apr 6, 2022
@dehann dehann moved this to Sufficient or Deferred in Code Fixes for ICRA 2022 Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Sufficient or Deferred
Development

No branches or pull requests

3 participants