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

Add non-size-specializing variants of manifolds #625

Closed
20 tasks
mateuszbaran opened this issue Jun 12, 2023 · 18 comments · Fixed by #642
Closed
20 tasks

Add non-size-specializing variants of manifolds #625

mateuszbaran opened this issue Jun 12, 2023 · 18 comments · Fixed by #642
Labels
enhancement New feature or request

Comments

@mateuszbaran
Copy link
Member

Currently most manifolds have size embedded in their type. That's nice for runtime performance but adds additional time for compilation which is sometimes an undesirable trade-off.

Manifolds that should have non-size-specializing variants:

  • AbstarctUnitaryMatrices
  • CenteredMatrices
  • Elliptope
  • FixedRankMatrices
  • Flag
  • GeneralizedGrassmann
  • GeneralizedStiefel
  • Grassmann
  • MultinomialMatrices
  • MultinomialSymmetric
  • Oblique
  • OrthogonalMatrices
  • ProbabilitySimplex
  • ProjectiveSpace
  • SphereSymmetricMatrices
  • Stiefel
  • SymmetricMatrices
  • SymmetricPositiveDefinite
  • SymmetricPositiveSemidefiniteFixedRank
  • Tucker
@mateuszbaran mateuszbaran added the enhancement New feature or request label Jun 12, 2023
@kellertuer
Copy link
Member

Can you give an example how we would provide both in a nice way?
For completeness (and me to understand); maybe with the sphere?

@mateuszbaran
Copy link
Member Author

For Sphere, we could do

struct DynSphere{𝔽} <: AbstractSphere{𝔽}
    N::Int
end
representation_size(M::DynSphere) = M.N
function get_embedding(M::DynSphere{𝔽}) where {𝔽}
    return DefaultManifold(representation_size(M)...; field=𝔽)
end

and more or less all other methods should work fine thanks to dispatching on AbstractSphere. We would then need something like AbstractSphere for other manifolds (AbstractStiefel etc.). For some manifolds it would be more important than for other, for example Grassmann and Stiefel look high-ish priority while OrthogonalMatrices would be less important IMO.

The big problem is coming up with reasonable naming. I think ideally the "plain" name should refer to the non-specializing variant, and something like StaticSphere should be the current specializing one but that would be slightly breaking.

@kellertuer
Copy link
Member

I still do not necessarily see the benefit of these new types.
They should have quite a benefit, since keeping both always means we have to carefully communicate that to the user.
If we can't decide which one is better and have two, how should the user decide?

@kellertuer
Copy link
Member

On the other side, if that is faster, we could also change all dimension-parameters from type parameters to fields for all manifolds (and do a breaking release)?
but then I would really like to see a benchmark ;)

@mateuszbaran
Copy link
Member Author

A lot less code would have to be compiled when using non-static sizes so it could significantly lower TTFX but that's very application-specific. Also, I think there are very few cases where there are optimization opportunities offered by encoding size in manifold (i.e., we can usually use size from statically sized points). Of course, it's slightly easier to exploit the size encoded in manifold as opposed to a point and that's how we currently do it but it has some drawbacks.

@kellertuer
Copy link
Member

kellertuer commented Jun 12, 2023

Hm, can we trick them to be in the same struct, without too much technicalities? Then the user does not have to care, the constructors would work as before but we have another keyword that switches both?

Something Like (just a first sketch of said idea)

abstract struct AbstractSizeType end
struct SizeVariable <: AbstractSizeType end
struct SizeParameter{D} <: AbstractSizeType end

and we can either store the size aas a field in SizeVariable or in the manifold.
The manifold would be something like

struct Sphere{S,T,𝔽} <: AbstractSphere{𝔽}
dims::T #T would be nothing if S is a SizeParameter
end

or if the dimension is a field of SizeVariable

struct Sphere{S<:AbstractSizeType,𝔽} <: AbstractSphere{𝔽}
size::S
end

S is either the SizeParameter (and we can dispatch on the dimension D) or the SizeVariable (and we can access its field).
For us that would maybe be a bit technical to figure out in nice, but it stays uncomplicated for the user

edit: hit submit too early ;) The question is of Course if this remains performant with both advantages you say. The advantage might be that a keyword argument (and even a single place to handle said keyword) could decide this for every manifold.
So for the user this stays simple.

@mateuszbaran
Copy link
Member Author

Yes, that should work fine.

@kellertuer
Copy link
Member

And yet again – I have not much clue about performance but sometimes ideas how to keep things simple for the user.

You optimise everything (and I am very grateful for that, so that together we find something that is hopefully fast and easy for the user :)

We could do a keyword like sizetype=:veriable | :parameter ?

@mateuszbaran
Copy link
Member Author

I'm not planning to work on it immediately but I wanted to open it to remember about this issue and discuss ideas for the interface, so thanks for the size parameter suggestion 👍 .

A keyword may be a good idea for keeping the selection simple.

@kellertuer
Copy link
Member

It also sounds like a bit of work, but good to have it in mind.

@mateuszbaran
Copy link
Member Author

I've started working on this and it won't really work as a non-breaking change. The one thing that is necessary to break is how size-specific overloads are done but honestly I think it's a good moment to consider moving towards non-size-specializing manifolds as the default. @kellertuer what do you think?

#642 is also breaking so I could finish both during the next few weeks and then we could have a big 0.9 release.

@kellertuer
Copy link
Member

Is fine with me, we could finally also rename the affine invariant metric.

@mateuszbaran
Copy link
Member Author

Cool, we might also take a look if there are any other breaking changes we intended to make.

@kellertuer
Copy link
Member

Yes, would be a good time for sure. Also I feel we kept 0.8 for a good while already, which for me points to we are getting more and more stable :) So we hopefully have not too many breaking ones in the future anyways.

@kellertuer
Copy link
Member

We could unify exp and retract, see JuliaManifolds/ManifoldsBase.jl#159 or even unify the isapproxnames as discussed JuliaManifolds/ManifoldsBase.jl#135

@kellertuer
Copy link
Member

Since the last large project I was aiming for is done (hessian convergence) I will work a bit on all the small things, and probably start with reworking the retract dispatch next week, so we might have a breaking change in ManifoldsBase soon-ish.

@mateuszbaran
Copy link
Member Author

OK, cool. The ManifoldsBase.jl part of dispatch rework should be fairly easy, and the Manifolds.jl part should go to #642 I think.

@kellertuer
Copy link
Member

That sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants