JuliaManifolds / ManifoldsBase.jl

Basic interface for manifolds in Julia
https://juliamanifolds.github.io/ManifoldsBase.jl/
MIT License
87 stars 8 forks source link

Convenience for real-coefficient bases construction #180

Closed kellertuer closed 8 months ago

kellertuer commented 9 months ago

Our basis could be improved in two ways

Improve documentation

Currently the documentation is not so clear about the field our bases carry. It should be made clear that the parameter refers to the number type if the vectors that span the basis.

For real manifolds this is of course not so important, but for complex ones there is an interesting distinction

This could and should be improved in the docs

Convenience in the basis constructors

Currently not providing an argument always yields the real-basis-vectors case, which is also not so nice on complex manifolds, see above, since it is counter intuitive. On the other hand, a basis can not directly guess what manifold it belongs to. Here is 2 ideas how to solve this

Convenience constructor (non-breaking)

Introduce DefaultBasis(M) which chooses the field from the manifold also for the basis and hence leads to the real-coefficients. One could even deprecate the empty constructor.

Switch the field meaning (highly breaking)

One could refer to the stored field as meaning the coefficients (and not the basis) number type. This has the advantage that real-valued-coefficients would always be the default and that is most-intuitive. The disadvantage is, that this would be highly breaking for all of the JuliaManifolds Ecosystem, so one should carefully check whether this is worth it.

Maybe there is also a third idea?

kellertuer commented 9 months ago

I just had a third idea, since one can easily mix the two szenarios up, we could also say the Basis(F) constructors are discouraged from now on and just have 2 keywords

Basis(; coordinates=F) and Basis(; vectors=F)

This would be consistent with the naming of the two functions involved, if you set both and they contradict each other, there is an error, maybe also only in the get_ functions.

We could then defined even on the abstract level that Basis(M) sets vectors= to the same as the field of M.

This way it is easier to document, consistent with the naming of the two functions, even independent of how we actually store it in the struct.

mateuszbaran commented 9 months ago

I don't think we actually need the two keywords. It would just make a mess of confusing dispatch problems. The convenience constructor is IMO the most important part of this issue. We really need clear and easy documentation for real-coefficient bases (the most common use case).

kellertuer commented 9 months ago

Fair enough, this was mainly an idea to make it easier for the user to specify what they want. If you feel that complicates things, then let‘s not do that :)