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

Provide default basis for local_metric functions and make the basis parameter hence optional #424

Open
kellertuer opened this issue Sep 11, 2021 · 5 comments
Labels
small issue ... hopefully

Comments

@kellertuer
Copy link
Member

Currently local_metric(M, p, B) requires a basis (similarly det_local_metric, inverse_local_metric and jacobian_local_metric). It would be nice (and not too difficult) to provide a reasonable default for this last parameter, e.g. the DefaultOrthogonalBasis().

The only thing one has to take into account is that this has to respect the transparency (similar to retract or vector_transport_to).

@kellertuer kellertuer added the small issue ... hopefully label Sep 11, 2021
@kellertuer kellertuer mentioned this issue Oct 11, 2021
15 tasks
@kellertuer
Copy link
Member Author

I am not 100% sure why I proposed this, since the other functions, like get_vector and get_coordinates also do not have a default for the basis.

So maybe it is not reasonable to have a default?

Otherwise, we should maybe introduce a function similar to default_retraction_method(::AbstractManifold), maybe default_tangent_bases(::AbstractManifold)? I would do that in base and use it for other methods as well – but it might be a fight with transparency anyways.

@mateuszbaran
Copy link
Member

Things like det_local_metric are not sensitive to change of basis so it would make sense to add a method that doesn't take a basis but overall this seems like a low priority issue. Anyway adding variants that don't take the basis seems like a lot of work for very little benefit.

Otherwise, we should maybe introduce a function similar to default_retraction_method(::AbstractManifold), maybe default_tangent_bases(::AbstractManifold)? I would do that in base and use it for other methods as well – but it might be a fight with transparency anyways.

DefaultBasis should usually be the default_tangent_basis for any manifold. We could prefer a different one sometimes but before touching bases I'd prefer to refactor them like here: JuliaManifolds/ManifoldsBase.jl#88 . I've tried doing that for an hour or so and things like ProductManifold were quite troublesome to deal with. Some ambiguities still remain, mostly because ProductManifold actually introduces complex logic there but I'd expect there to be much less ambiguity fighting.

@kellertuer
Copy link
Member Author

ok, so we can go for `DefaultBasis, sure. But should this issue wait for the base one to be finished?

@mateuszbaran
Copy link
Member

I personally wouldn't do any restructuring around bases without solving that ManifoldsBase issue first, at least the part relevant to bases.

@kellertuer
Copy link
Member Author

ok, then lets wait with this one until the other is finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
small issue ... hopefully
Projects
None yet
Development

No branches or pull requests

2 participants