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

UTF8 Documentation rework #104

Merged
merged 79 commits into from
Feb 5, 2020
Merged

UTF8 Documentation rework #104

merged 79 commits into from
Feb 5, 2020

Conversation

kellertuer
Copy link
Member

Works on #101 and introduces a notation overview. Here's the current details

We can not to anything against

  • \mathrm{M}, \mathrm{H}, \mathrm{T} (for the Minkowski-norm, Hermitian and transposed vectors),...
  • the same for \operatornames
  • \lvert and \rvert (as well as \lVert and \rVert) are hard to replace, since the utf8 symbols (| and ‖) have in TeX a different spacing

I replaced

  • all manifolds (M, \mathcal M) in math mode with ℳ
  • \langle, \rangle with their utf8s ⟨⟩
  • \cdot with
  • \ldots with
  • \times with ⨉

I noticed that the links to allocate, allocate_resultandnumber_eltypereturn warnings – are they properly exported in ManifoldsBase`?

Within the table on notations.md on my machine it seems random whether math is recognized or not... can someone verify that? My change from $ to `` dioes only randomly take effect.

What do you think to the proposed notation? Some of that is really not unified currently (especially dot/inner and (co)tangents).

@kellertuer
Copy link
Member Author

A first note is – despite the notation.md rendering all renders fine on safari. Concerning the unified notation, I'd like some feedback on what we agree – document that in notation.md and then unify the decstrings accordingly.

src/statistics.jl Outdated Show resolved Hide resolved
@mateuszbaran
Copy link
Member

It looks quite nice in github file preview. I'll check it more closely later.

I noticed that the links to allocate, allocate_resultandnumber_eltypereturn warnings – are they properly exported in ManifoldsBase`?

allocate and number_eltype are exported in both ManifoldsBase and Manifolds. I don't know why there are warnings.

What do you think to the proposed notation? Some of that is really not unified currently (especially dot/inner and (co)tangents).

It's OK. One small issue is that I'm used to denoting tangent vectors by lower case letters (u, v, w) but I can use your notation here.

@kellertuer
Copy link
Member Author

It's OK. One small issue is that I'm used to denoting tangent vectors by lower case letters (u, v, w) but I can use your notation here.

i am not saying my proposal should definetly stay unchanged, it's just close to what I am used to (despite that we currently write ( , )_x for the inner product since theres also a dual pairing between tangent and cotangent). I do think it's nicer to have different sets for points & tangents.

@codecov
Copy link

codecov bot commented Jan 25, 2020

Codecov Report

Merging #104 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #104   +/-   ##
=======================================
  Coverage   94.29%   94.29%           
=======================================
  Files          44       44           
  Lines        2715     2715           
=======================================
  Hits         2560     2560           
  Misses        155      155           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bef2c9...4bef2c9. Read the comment docs.

Copy link
Member

@sethaxen sethaxen left a comment

Choose a reason for hiding this comment

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

Thanks for getting started on this! I have a few general comments.
Set notation, greek letters, and arrows can be converted to unicode equivalents.
RE × and , these sometimes end up looking really cramped. This is especially the case for the latter, which is why I didn't end up using it.
I personally don't like the look of the script characters, which are much more slanted than their equivalent mathcal.
There are a number of places where the old latex forms are still used. I noted a few but not all of them; you may need to grep to make sure none were missed.

docs/src/manifolds/product.md Outdated Show resolved Hide resolved
docs/src/manifolds/product.md Outdated Show resolved Hide resolved
docs/src/manifolds/symmetricpositivedefinite.md Outdated Show resolved Hide resolved
docs/src/notation.md Outdated Show resolved Hide resolved
docs/src/notation.md Outdated Show resolved Hide resolved
docs/src/notation.md Outdated Show resolved Hide resolved
src/groups/group.jl Outdated Show resolved Hide resolved
src/groups/special_euclidean.jl Show resolved Hide resolved
src/manifolds/CholeskySpace.jl Outdated Show resolved Hide resolved
src/manifolds/Hyperbolic.jl Outdated Show resolved Hide resolved
@kellertuer
Copy link
Member Author

For the mathcals, \times and \circ I will change that back then. Yes, I was also not so happy with those after changing them.

@kellertuer
Copy link
Member Author

Sometimes markdown (especially tables) math and utf8 within that seems to work not that nicely. Then I remembered that for notations.md we never show that in REPL anyways, so for the markdown files I propose to use classical LaTeX and just for the docstrings the UTF8

@kellertuer
Copy link
Member Author

Further, I would like to unify notations, the table is my proposal of what I noticed, feel free to suggest further notations and check whether you agree with my proposal. We can of course discuss, With a finished unified notation I would adapt the notation accordingly in the docstrings, where we currently have 3-4 different notations.

@kellertuer
Copy link
Member Author

Thanks for so carefully revising my proposed unification. It was actually some work; after finishing here I will to the same to base, also introducing code blue there'n stuff. I think I already reworked all your changes – noticed that the proposed changes (thanks for directly doing those) can be batch-commited when going through them in the files changed tab :)

Just two questions remain I think

  • Our differential notation from Rotations and the group manifolds
  • Our translation notation τ from group manifolds

Can you elaborate on those two? I added \circ to the notations. Is that what you meant with the first? The τ I could not find within notations for group manifolds.

@sethaxen
Copy link
Member

sethaxen commented Feb 4, 2020

Thanks for so carefully revising my proposed unification.

Thanks for doing this!

Just two questions remain I think

  • Our differential notation from Rotations and the group manifolds
  • Our translation notation τ from group manifolds

Can you elaborate on those two? I added \circ to the notations. Is that what you meant with the first? The τ I could not find within notations for group manifolds.

\circ in the notations is nice. There are also other operations that could go there in addition to the inner product, e.g. \times as a Cartesian product.

What I meant here was the differential in the sense used for translate_diff and apply_diff. It would be nice if we gave a more precise definition, but I think here it's just important to note the notation.

Also τ appears as a generalization of L and R, the left or right translations. It appears in

but it is used extensively in docstrings in #96. We should probably use it more extensively in translate, inverse_translate, apply, etc. If you'd prefer, I could handle this docstring update in #96 once this is merged.

I'll continue the review where I left off yesterday.

Copy link
Member

@sethaxen sethaxen left a comment

Choose a reason for hiding this comment

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

Whew! Okay I noted a bunch more changes. Some of them appear in more places including lines not yet edited, so I'll list some general observations here:

  • We need notation for basis vectors (and potentially separately for Ξ if not the same)
  • Notation for coefficients of a tangent vector in a basis (we sometimes use X^i, which I like)
  • We sometimes use tr and sometimes trace for trace, we should pick one
  • There are a number of places where v was changed to X, but the norm or square, for example, of X still has v in the name, so that the code is harder to follow I noted a few, but there were more. (e.g. vout).
  • transpose and conjugate transpose are both still missing from notations table (see previous review)
  • identity matrix and zero matrix should be in the notation table, and we should choose a convention whether they are bold or non-bold, mathrm or not (see e.g. Stiefel and Rotations). I generally like them non-bold and non mathrm.
  • "eminating" was in a bunch of places and should be "emanating" everywhere
  • \exp_pX -> \exp_p X for clarity. Likewise \log_pq -> \log_p q.

I probably won't have the time to review again for a few days, so I'll mark approved, and feel free to merge when you are satisfied. Thanks again!

docs/src/notation.md Outdated Show resolved Hide resolved
| $\mathcal M$ | a manifold | $\mathcal M_1, \mathcal M_2,\ldots,\mathcal N$ | |
| $\operatorname{Exp}$ | the matrix exponential | |
| $\operatorname{Log}$ | the matrix logarithm | |
| $\mathcal P_{q\gets p}X$ | parallel transport | | of the vector $X$ from $T_p\mathcal M$ to $T_q\mathcal M$
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the use of this notation vs P_{p \to q} ? The latter seems to read better to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats easy, if you concatenate parallel transports $P_{q\gets p}P_{p\gets s}X$, i.e. you transport X from s -> p -> q this version is far better readable than $P_{p\to q}P_{s\to p}$ since for this the order is obscured. The first one can be read just from right to left.

src/manifolds/ProductManifold.jl Outdated Show resolved Hide resolved
src/manifolds/Rotations.jl Outdated Show resolved Hide resolved
src/manifolds/Rotations.jl Outdated Show resolved Hide resolved
src/manifolds/SymmetricPositiveDefiniteLinearAffine.jl Outdated Show resolved Hide resolved
src/manifolds/SymmetricPositiveDefiniteLogCholesky.jl Outdated Show resolved Hide resolved
src/orthonormal_bases.jl Outdated Show resolved Hide resolved
src/statistics.jl Outdated Show resolved Hide resolved
ytmp = copy(yold)
v = zero_tangent_vector(M, y)
v = zero_tangent_vector(M, q)
Copy link
Member

Choose a reason for hiding this comment

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

v to X in this file. Probably useful to change x to p as elsewhere to avoid confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left x when it was a vector of points on the manifold. but I changed x0 to p0.

@kellertuer
Copy link
Member Author

Additionally to $\times$ I also added $\hat$ for the Cartesian (n-ary) power.

It would be great if you could add $\tau$ within your #96 then, when you use
it from then on more often anyways.

I tried to find further v-resemblemses not that I also did not (xet) rename
all tests and their variables.

For the matrices I also prefer non-bold non-mathrm, just $0_k$ and $I_k$ and added that to the notations as well.

Basis vectors are just $\Xi = {X_1,\ldots,X_n}$, i.e. $\Xi$ is a set of vectors, maybe a basis. I thought we use $X_1$ and $X^1$ more like depending on the Einstein summation? This might not yet be fully unified.

Finally, I don't know how I came to "eminating" I corrected those occurences.

I will keep this one open a day or two just to give everybody a possibility to take a final look.

| $^{\wedge}$ | (n-ary) Cartesian power of a manifold | | see [`PowerManifold`](@ref) |
| $T^*_p \mathcal M$ | the cotangent space at $p$ | | |
| $\xi$ | a cotangent vector from $T^*_p \mathcal M$ | $\xi_1, \xi_2,\ldots,\eta,\zeta$ | sometimes written with base point $\xi_p$. |
| $n$ | dimension (of a manifold) | $n_1,n_2,\ldots,m, \operatorname{dim}(\mathcal M)$| for the real dimension sometimes also $\operatorname{dim}_{\mathbb R}(\mathcal M)$|
Copy link
Member

Choose a reason for hiding this comment

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

In docstrings \dim is used instead of \operatorname{dim}. Is this due to a documentation rendering issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's mainly due to me having missed to replace those, after I noticed \dim is available. I replaced those two, too, now.

@kellertuer kellertuer merged commit a56963f into master Feb 5, 2020
@kellertuer kellertuer deleted the documentation-rework branch February 5, 2020 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants