Documentation and Convenience wrappers #5
Conversation
README.md
Outdated
| To combine Manifolds one can use `PowerManifold` and `ProductManifold`. | ||
| The constructor of `PowerManifold` takes the exponentiated manifold `M`, the dimensions of this manifold `inner_dims` and the exponents `outer_dims`. | ||
| The constructor of `ProductManifold` takes the two manifolds `m1` and `m2` and their respective dimensions `dims1` and `dims2` as arguments. | ||
|
|
There was a problem hiding this comment.
I'd rather not document those yet, as they have a pretty crappy interface. Let those who want it use it in wait of a better code.
| @@ -2,6 +2,21 @@ A Julia library to handle projections on manifolds. This is useful for minimizin | |||
|
|
|||
| Currently, the sphere `{x ∈ K^n, ||x|| = r}` and the Stiefel manifold `{X ∈ K^{n × m}, X'*X = I}` as well as independent copies of these manifolds are supported. | |||
|
|
|||
| The projections implemented are retract and project_tangent (both are also available inplace `!`). | |||
| ``` | |||
| retract(M::Manifold, x) = retract!(M, copy(x)) | |||
There was a problem hiding this comment.
No need to say how retract is implemented to the user. Maybe just say (after the "example" section) how to define a new manifold, which would function as an API documentation? Taking for instance the example of the "partial sphere" you had.
src/ManifoldProjections.jl
Outdated
| retract(M::Manifold, x) = retract!(M, copy(x)) | ||
| "Retracts a given point `x` back onto the Manifold `M`" | ||
| function retract!(M::Manifold, x) end |
There was a problem hiding this comment.
IMO this type of comment is just noise: it doesn't tell you anything that you wouldn't get by reading the function name. Adding more comments is great when a code is finalized and won't change for ten years. I doubt this is the case with this library, so any comment means giving a false sense of a mature library, and means having to maintain it when we change things around.
src/ManifoldProjections.jl
Outdated
| function retract!(M::Manifold, x) end | ||
| """ | ||
| Returns the projection of the given vector `g` into the tangent space on the Manifold `M` around the point `x`. | ||
| `x` is assumed to lie on the manifold. This is not checked! |
There was a problem hiding this comment.
"around the point x (assumed to lie on M)"
src/ManifoldProjections.jl
Outdated
| @@ -145,6 +160,9 @@ struct ProductManifold<:Manifold | |||
| dims1::Tuple | |||
| dims2::Tuple | |||
| end | |||
| ProductManifold(m1::Manifold, m2::Manifold, dim1::Int, dim2::Int) = ProductManifold(m1, m2, Tuple(dim1), Tuple(dim2)) | |||
| ProductManifold(m1::Manifold, m2::Manifold, dim1::Tuple, dim2::Int) = ProductManifold(m1, m2, dim1, Tuple(dim2)) | |||
| ProductManifold(m1::Manifold, m2::Manifold, dim1::Int, dim2::Tuple) = ProductManifold(m1, m2, Tuple(dim1), dim2) | |||
There was a problem hiding this comment.
I'd rather not have these for the same reason as above: this API is pretty bad anyway...
|
Thanks for the PR! See comments. Sorry to be so negative, but this library is a newborn (it's not even used by anybody yet), and adding convenience wrappers and comments, while great in theory, feels premature at this stage. |
|
It is fine, its better to say clear what you envision |
A few convenience wrappers for the product and power manifold contructors.
But primarily some documentation of the functions.
Is it OK to add empty functions on the abstract type, that contain the documentations, as all retract and project_tangent have the same basic functionality?
Also added a short description of most things in the readme.
If you have any feedback let me know