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

Poincaré models for Hyperbolic space #219

Merged
merged 40 commits into from
Sep 22, 2020

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Sep 10, 2020

I started testing how different formats for the same manifold work, i.e. besides the arrays, which are assumed to be form the hyperbolic model implicitly there is now

  • the Hyperboloid model
  • the Poincaré ball model
  • the Poincaré half plane model (maybe better to name it half space)?

Where all have types for their MPoints and TVectors. All concerning converts have been implemented. Still todo

  • exp/log/inner for the plane model
  • check which models do not directly work, especially those that originally came from the embedding (which only holds for the first model)
  • check whether we have to extend the allocate_result functions
  • add tests.
  • convert tangent vectors

@kellertuer kellertuer added the WIP Work in Progress (for a pull request) label Sep 10, 2020
@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #219 into master will decrease coverage by 0.32%.
The diff coverage is 91.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
- Coverage   95.50%   95.17%   -0.33%     
==========================================
  Files          65       69       +4     
  Lines        4089     4248     +159     
==========================================
+ Hits         3905     4043     +138     
- Misses        184      205      +21     
Impacted Files Coverage Δ
src/Manifolds.jl 100.00% <ø> (ø)
src/groups/group.jl 76.96% <0.00%> (-0.82%) ⬇️
src/manifolds/Stiefel.jl 92.72% <ø> (ø)
src/manifolds/Hyperbolic.jl 89.09% <91.30%> (+3.72%) ⬆️
src/manifolds/HyperbolicHyperboloid.jl 92.85% <92.85%> (ø)
src/manifolds/HyperbolicPoincareBall.jl 95.00% <95.00%> (ø)
src/manifolds/HyperbolicPoincareHalfspace.jl 95.00% <95.00%> (ø)
src/statistics.jl 99.42% <100.00%> (ø)
src/tests/tests_general.jl 98.54% <100.00%> (ø)
src/manifolds/SymmetricPositiveDefinite.jl 93.10% <0.00%> (-6.90%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88037e7...839e122. Read the comment docs.

@kellertuer
Copy link
Member Author

kellertuer commented Sep 11, 2020

I noticed something interesting:

While to define your own allocate_result boils down to defining a function for allocate_result(M, log, p, q), i.e. arguments after the function name),
for allocate_result_type one has to define allocate_result_type(M, log, (p,q)), i.e. putting the arguments in a tuple. Would it be reasonable to unify that to the first? Makes defining those nicer. But actually it only affects log/inverse_retract since they are specific in the type, since from two MPoints we have to specify the fitting TVector.
Maybe that could be globally done by defining a map from MPoint to TVector for these types?

So maybe something like vector_type(::Type{T}) where {T} = T for the general case and for specific own types vector_type(::Type{HyperboloidPoint}) = HyperboloidTVector for example? this would collapse a lot of my current allocation type definitions.

@mateuszbaran
Copy link
Member

That's a good idea. There may be some minor things to work out though. Type conversion would have to be more precise (forwarding the type argument of the wrapper) like vector_type(::Type{HyperboloidPoint{T}}) where {T} = HyperboloidTVector{T} (not tested).

@kellertuer
Copy link
Member Author

So now the main thing missing might be either look for log/exp in the different 2 other models or derive conversions for tangent vectors accordingly (taking the derivative of the conversion functions). And it might be worth doing both.

@kellertuer
Copy link
Member Author

despite two conversions being done as double conversions, one conversion is missing, namely how to transform Poincaré ball tangent vectors to Hyperboloid ones. Still have to look for that formula (or derive it from the push forward of the isometry myself).

@kellertuer
Copy link
Member Author

All methods should now be implemented, all formula documented (derived the last one by hand just now); still have to check whether the remaining exp/log/retract methods are failing due to a typo in the formulae or whether that's numerical instability.

@kellertuer
Copy link
Member Author

It seems, that conversion between both Poincaré models still has a bug.

@kellertuer
Copy link
Member Author

There is so much converting going on... I'll take a closer look at it later.

I convert everything to everything (and its 4 types) as well as points to points, point/vector to vector and point&vector to point&vector. We can omit the last case if that is not needed, especially if I change the point/vector->vector interface now anyways.

Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK now 👍 .

@mateuszbaran
Copy link
Member

Oh well... it fails on MacOS but not Ubuntu or Windows...

@mateuszbaran
Copy link
Member

I think it works now 🎉.

@kellertuer kellertuer merged commit 360b66b into master Sep 22, 2020
@kellertuer kellertuer removed the Ready-for-Review A label for pull requests that are feature-ready label Sep 22, 2020
@kellertuer kellertuer deleted the kellertuer/HyperbolicRepresentations branch September 25, 2020 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants