Replace DefaultManifold by Euclidean in ManifoldsBase.jl

[juliohm]

It would be really convenient for developers who depend on ManifoldsBase.jl if Euclidean was readily available as the actual default manifold. Sometimes we need to implement fallback methods that return Euclidean, and can't afford Manifolds.jl as a dependency.

I believe that DefaultManifold aspires to be Euclidean, and that it could be removed for practical purposes.

Would you consider moving the type here, and reexporting in Manifolds.jl? It should be a breaking change in ManifoldsBase.jl with little effect on end-users of the ecosystem.

[mateuszbaran]

IIRC we had Euclidean here at some point but it was removed (see #6 by @kellertuer). The static dimension part has been reworked since then and my justification from that issue no longer applies.

@kellertuer , what do you think about it?

[kellertuer]

I would still follow and prefer my argument from back then,
additionally to the arguments back then

• Euclidean is currently implemented in one place, covering all functions from ManifoldsBase here and a few that are in Manifolds but for different reasons not or not yet in ManifoldsBase
• I do like that the DefaultManifold is sparse also in documentation, and Euclidean (in the library of manifold) richly documented.

So I personally would prefer the current state, though I do agree, I currently added a small function to have Rn(n) map to DefaultManifold in general but use Euclidean if Manifolds is loaded, see
https://github.com/JuliaManifolds/Manopt.jl/blob/465b0b77e9868efbcc4d131a8e0cabb0c8da2301/src/Manopt.jl#L152-L169
and the modification when extending it, here
https://github.com/JuliaManifolds/Manopt.jl/blob/465b0b77e9868efbcc4d131a8e0cabb0c8da2301/ext/ManoptManifoldsExt/ManoptManifoldsExt.jl#L29

I would be fine if we do a similar function (and extension) here that DefaultManifold gets a small additional way to create it that maps to Euclidean if one loads manifolds (I would then use this new one in Manopt even as well).

[juliohm]

• Euclidean is currently implemented in one place, covering all functions from ManifoldsBase here and a few that are in Manifolds but for different reasons not or not yet in ManifoldsBase

That seems like an opportunity to polish the ManifoldsBase.jl interface with more functions that are only defined in Manifolds.jl.

• I do like that the DefaultManifold is sparse also in documentation, and Euclidean (in the library of manifold) richly documented.

I don't understand how this helps users of the Manifolds.jl ecosystem. Wouldn't it be more user-friendly if a complete, well-documented implementation is available by default? I don't see much benefit in maintaining a DefaultManifold that is sparse in documentation, which is likely not used in practice.

My understanding from your reply is that you agree that moving Euclidean back here is beneficial to the Manifolds.jl user base, and that small helper functions like Rn(n) would not be necessary in this case. Is my interpretation of your reply correct? How can I help make this happen?

[kellertuer]

• Euclidean is currently implemented in one place, covering all functions from ManifoldsBase here and a few that are in Manifolds but for different reasons not or not yet in ManifoldsBase

That seems like an opportunity to polish the ManifoldsBase.jl interface with more functions that are only defined in Manifolds.jl.

We move functions over to ManifoldsBase when we either have a good reason to or when we agree that their function (name, signature,...) has matured enough. So we do that regularly, but we also regularly introduce new functions. So that is what we do anyways, but there are usually still always new, not-yet-so-mature functions only defined in Manifolds.jl for now; moving them too early would lead to possibly more breaking changes (not mature enough -> they needed some further polishing still), which I would like to avoid as much as possible

• I do like that the DefaultManifold is sparse also in documentation, and Euclidean (in the library of manifold) richly documented.

I don't understand how this helps users of the Manifolds.jl ecosystem. Wouldn't it be more user-friendly if a complete, well-documented implementation is available by default? I don't see much benefit in maintaining a DefaultManifold that is sparse in documentation, which is likely not used in practice.

DefaultManifold is not exported, so I personally consider that an internal detail of ManifoldBase. I would even be ok moving it to for example a ManifoldsBaseTest extension, because that is the reason why it is defined – to have a manifold to test the interface against (as you can see above we introduced this 2019 quite some time before extensions existed) – or into a TestSubpackage that is added on test time.

My understanding from your reply is that you agree that moving Euclidean back here is beneficial to the Manifolds.jl user base, and that small helper functions like Rn(n) would not be necessary in this case. Is my interpretation of your reply correct? How can I help make this happen?

No. Then you misread my conclusion. I am still against moving it. I do not see the benefit for it. What I did write was, that the Rn function currently defined in Manopt (or maybe soon defined there to be precise, that is on a PR/branch) could be something we could move here to make it easier to get “some vector space”; but for now I really only have exactly one use case for that in the forthcoming solver from that branch.

So in conclusion: ManifoldsBase.jl defines an interface and only that. With the discussion here I would be more open to even move DefaultManifold to an extension or a sub-module that is only loaded in testing.

[juliohm]

So if someone needs R^n as a fallback, that requires depending on the full Manifolds.jl and its various dependencies? Moving DefaultManifold to a test dependency would worsen the situation because then no fallback would be readily available. Em qua., 17 de jul. de 2024, 18:54, Ronny Bergmann ***@***.***> escreveu:

…

- Euclidean is currently implemented in one place, covering all functions from ManifoldsBase here and a few that are in Manifolds but for different reasons not or not yet in ManifoldsBase That seems like an opportunity to polish the ManifoldsBase.jl interface with more functions that are only defined in Manifolds.jl. We move functions over to ManifoldsBase when we either have a good reason to or when we agree that their function (name, signature,...) has matured enough. So we do that regularly, but we also regularly introduce new functions. So that is what we do anyways, but there are usually still always new, not-yet-so-mature functions only defined in Manifolds.jl for now; moving them too early would lead to possibly more breaking changes (not mature enough -> they needed some further polishing still), which I would like to avoid as much as possible - I do like that the DefaultManifold is sparse also in documentation, and Euclidean (in the library of manifold) richly documented. I don't understand how this helps users of the Manifolds.jl ecosystem. Wouldn't it be more user-friendly if a complete, well-documented implementation is available by default? I don't see much benefit in maintaining a DefaultManifold that is sparse in documentation, which is likely not used in practice. DefaultManifold is not exported, so I personally consider that an internal detail of ManifoldBase. I would even be ok moving it to for example a ManifoldsBaseTest extension, because that is the reason why it is defined – to have a manifold to test the interface against (as you can see above we introduced this 2019 quite some time before extensions existed) – or into a TestSubpackage that is added on test time. My understanding from your reply is that you agree that moving Euclidean back here is beneficial to the Manifolds.jl user base, and that small helper functions like Rn(n) would not be necessary in this case. Is my interpretation of your reply correct? How can I help make this happen? No. Then you misread my conclusion. I am still against moving it. I do not see the benefit for it. What I did write was, that the Rn function currently defined in Manopt (or maybe soon defined there to be precise, that is on a PR/branch) could be something we could move here to make it easier to get “some vector space”; but for now I really only have exactly one use case for that in the forthcoming solver from that branch. So in conclusion: ManifoldsBase.jl defines *an interface* and only that. With the discussion here I would be more open to even move DefaultManifold to an extension or a sub-module that is only loaded in testing. — Reply to this email directly, view it on GitHub <#194 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZQW3OEKXKZMC76SQG4PFDZM3RY7AVCNFSM6AAAAABLAXMYE6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZUGM4DOMBYGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[kellertuer]

The more arguments and the more I think about it, the more I want to move even Defaultmanifold out of the abstract definition and ideas of an interface, which is the main focus of this package.

ManifoldsBase.jl is not meant to provide actual implementations of manifolds. Even DefaultManiolf was really only defined here to be able to test the interface. It is not exported and should not be considered being used – or available here.

Why would you need such a fallback?

In my case above I will move the availability of the default value I fill with the ProductManifold(M, Euclidean(n)) to the ManoptManifoldsExt, that is where it belongs, we can only provide a fallback to Euclidean if Euclidean exists – and concrete manifolds are defined in Manifolds.jl – a Library on manifolds, which is even its slogan.

Again, also your new arguments convince me more and more to move this internal (not part of the public API) part, the DefaultManifold into an ManifoldsBaseTestExt.

So the answer to “So if...” is – then use a weak dependency, or define your own Euclidean-like manifold (or the parts you need of the Euclidean) yourself.
Where would you need that?

[juliohm]

As explained on Zulip, I disagree with the viewpoint that a Base package should not provide canonical types. This is not useful for developers who need a fallback or ways to convert between custom and canonical representations. Multiple interface packages in Julia do that. Tables.jl for example comes with built-in table types for convenience. The net result of this purist view in ManifoldsBase.jl is that users will simply copy/paste their own Euclidean manifold to avoid depending on the full Manifolds.jl package. Em qui., 18 de jul. de 2024, 01:10, Ronny Bergmann ***@***.***> escreveu:

…

The more arguments and the more I think about it, the more I want to move even Defaultmanifold out of the abstract definition and ideas of an interface, which is the main focus of this package. ManifoldsBase.jl is not meant to provide actual implementations of manifolds. Even DefaultManiolf was really only defined here to be able to *test* the interface. It is not exported and should not be considered being used – or available here. Why would you need such a fallback? In my case above I will move the availability of the default value I fill with the ProductManifold(M, Euclidean(n)) to the ManoptManifoldsExt, that is where it belongs, we can only provide a fallback to Euclidean if Euclidean exists – and concrete manifolds are defined in Manifolds.jl – a Library on manifolds, which is even its slogan. Again, also your new arguments convince me more and more to move this internal (not part of the public API) part, the DefaultManifold into an ManifoldsBaseTestExt. So the answer to “So if...” is – then use a weak dependency, or define your own Euclidean-like manifold (or the parts you need of the Euclidean) yourself. Where would you need that? — Reply to this email directly, view it on GitHub <#194 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZQW3K2YS3PEO7SVG2VPTTZM452BAVCNFSM6AAAAABLAXMYE6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZVGI4TCOBUHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[mateuszbaran]

Both of you make valid points and personally I'm a bit indifferent about moving Euclidean to ManifoldsBase. There are ways around that type not being here such as making a package extension or copying its definition and adapting it to your needs, so I won't push for moving Euclidean here.

On a more positive note, there are plans to make Manifolds.jl significantly lighter by moving some hard dependencies to package extensions.

[kellertuer]

Yes, making Manifolds.jl a bit lighter might be the better way to go indeed.

I do agree, that there are arguments against my opinion as well, sure, I personally would prefer to keep ManifoldsBase.jl really to be the interface definitions. Maybe at some point defining an interface might need Euclidean (but hopefully never something concrete beyond that); then it might be okay-ish. Until there is no need, I would prefer the current state – maybe even moving DefaultManifold really just into the tests to further emphasise the API nature of this package.