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

Poincare ball model #45

Merged
merged 82 commits into from Mar 31, 2019
Merged

Poincare ball model #45

merged 82 commits into from Mar 31, 2019

Conversation

ferrine
Copy link
Member

@ferrine ferrine commented Feb 13, 2019

Huray, we are ready to start.

image
Image from here

Interesting reading I've done so far:

Hyperbolic Networks
Hyperbolic Entailment Cones for Learning Hierarchical Embeddings
Poincaré GloVe: Hyperbolic Word Embeddings

Some implementation takeouts (mostly from here):

  • Project results of all operations in the ball of radius 1 − eps, where eps = 10^{-5}
  • Numerical errors also appear when hyperbolic vectors get closer to 0, perturb with eps = 10^{-15}
  • Pass clipped to [-15, 15] input to tanh and clip tanh^{-1} to [-1+eps, 1-eps]

CC @leuchine!

@ferrine ferrine changed the title add base Poincare ball model Feb 13, 2019
@leuchine
Copy link

Thanks! @ferrine This is very insightful!

@KhrulkovV
Copy link

KhrulkovV commented Feb 16, 2019

Very interesting, perhaps we can add more models (Lorentz hyperboloid model which is more stable for numerical optimization) later?

https://arxiv.org/abs/1806.03417

@ferrine
Copy link
Member Author

ferrine commented Feb 16, 2019

@KhrulkovV Hi! Happy to see you there. I'll check that paper!

@ferrine
Copy link
Member Author

ferrine commented Feb 17, 2019

I've tried to write some tests for parallel transport in Poincare ball model and found it a frustrating idea. Parallel transport is very numerically unstable and sometimes gives ridiculous errors. Sometimes passing tests and sometimes not. Trying to see what happens, I've plotted it in matplotlib and it was visually ok and perhaps makes little sense.
image

BTW, I found a way to add figures to docs, it will be neat

Updates: I found some bugs

Updates 1: bugs fixed!!!!
image

@ferrine
Copy link
Member Author

ferrine commented Feb 19, 2019

I'm done. There are some docs+implementation. Waiting for tests pass now

@ferrine
Copy link
Member Author

ferrine commented Feb 21, 2019

Tests with adam do not work if I use torch script with nontensor arguments, as:
pytorch/pytorch#14455

@leuchine
Copy link

Hi @ferrine . Thanks for taking your efforts to implement the Poincare model. Here is another paper on Lorentz model, which may be of interests to you. https://arxiv.org/pdf/1902.00913.pdf

Also, may I ask that do you mind sharing your scripts for drawing the tests for parallel transport in Poincare ball model? I plan to use it for testing my own parallel transport implementation. Thanks!

Best Regards,
Qi

@ferrine
Copy link
Member Author

ferrine commented Feb 25, 2019 via email

@leuchine
Copy link

Thanks! @ferrine This is very helpful.

@ferrine
Copy link
Member Author

ferrine commented Mar 7, 2019

Could anyone of you review docs here? Maybe @leuchine or @newkozlukov if you have free time?

The checklist is the following:

  • Docstring is rendered correctly
  • Docstring is easy to read
  • Plots are easy to read, they contain enough information to get a better intuition. (I'm fine to redo them)
  • There are no typos
  • There are no math typos
  • Name conventions are held, the function name is not misleading and its arguments are self explained

If Any of these points are violated, please write a short comment in the corresponding place. I believe this is the way to create better docs.

@leuchine
Copy link

leuchine commented Mar 7, 2019

Thanks! @ferrine I will try to read it this week.

@leuchine
Copy link

leuchine commented Mar 7, 2019

I just glanced through. The doc looks great. I will continue later.

A Typo: Exp, Log -> exp, log

BTW, the Lorentz model including exp, log and parallel transport has already been implemented here. @ferrine @KhrulkovV . https://github.com/facebookresearch/poincare-embeddings/blob/master/hype/lorentz.py
Also, as there are mapping functions from Poincare to Lorentz and vice versa, these code might be reused. Thanks!

@KhrulkovV
Copy link

There is a small typo in docs: distance on hyperboloid is arccosh(x, y), not arccos(x, y). Will read more carefully later.

@ferrine
Copy link
Member Author

ferrine commented Mar 7, 2019

@leuchine

A Typo: Exp, Log -> exp, log

I used a capital letter to distinguish this operation from regular exp/log. Is it a convention to use lowercase?

The code from FB is great. I should take their best practice implementations using a Function class. It seems to help with numerical stability

@leuchine
Copy link

leuchine commented Mar 7, 2019

@ferrine It is not a big problem. Just a small convention in math publications I think. :) Low priority. Thanks!

@ferrine
Copy link
Member Author

ferrine commented Mar 15, 2019

Any additional comments, guys? Eager to continue the PR

@ferrine
Copy link
Member Author

ferrine commented Mar 17, 2019

I've observed weird behavior in torch.jit.trace (#57) rebased to follow master branch

Copy link
Member

@rrkarim rrkarim left a comment

Choose a reason for hiding this comment

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

I'm approving it for now with some uncertainty. More analysis is required for this branch.

@ferrine
Copy link
Member Author

ferrine commented Mar 22, 2019 via email

@ferrine
Copy link
Member Author

ferrine commented Mar 24, 2019

@leuchine, @KhrulkovV going to finish this in 5hrs today. If you have any comments, this is the best time to write them.

@ferrine
Copy link
Member Author

ferrine commented Mar 24, 2019

todos for today:

@ferrine
Copy link
Member Author

ferrine commented Mar 24, 2019

I faced numerical issues implementing #59, I have concerns about this change

@ferrine
Copy link
Member Author

ferrine commented Mar 25, 2019

@newkozlukov please approve that you allow my merging this PR without changing the parametrization as you proposed in #59. And you will pick my work up on doing this some time. I approve this change (#59) and if done before 1.0.0 can go without backward compatible code. Once you approve this, I finish this PR and switch to others.

@SomeoneSerge SomeoneSerge self-requested a review March 28, 2019 07:39
Copy link

@SomeoneSerge SomeoneSerge left a comment

Choose a reason for hiding this comment

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

As this has been already too long and blocked many other activities, I'm approving this with assumption that #59 will be fixed in future PRs. I still insist that gyrovector formalism be either removed from the documents or introduced more gradually on per-example basis -- again, in future PRs.

@ferrine ferrine merged commit 9d5ff25 into master Mar 31, 2019
@ferrine
Copy link
Member Author

ferrine commented Mar 31, 2019

Thank you all!

@ferrine ferrine deleted the poincare branch March 31, 2019 21:48
andbloch pushed a commit to andbloch/geoopt that referenced this pull request Dec 29, 2019
* add base

* add mobius add|sub

* fix

* missing formulas

* remove unused import

* add scalar mul, test props

* unnessesary cons in project

* no cover script functions

* add distance

* fix typo in comment

* add geodesics

* add expmap

* add functions

* add singlt apply

* black

* fix typos in docs

* fix typos in docs

* add parallel transport

* add dist to a plane and parallel transport. Parallel transport is numerically unstable

* fix math bugs

* add cool plots

* fix small things

* add egrad2rgrad

* add reference

* docs

* fix typos

* finish Poincare ball implementation

* fix small typo

* add to inifinite and beyond test

* add signed distance

* infinity and beyond test

* black

* docfix

* fix docs

* fix doc

* fix docs typos

* add import

* add dist0

* optim fails

* fix numerics, do not repare broken test

* black

* some refactoring

* fix typo

* p.data -> p in optim

* update docs a bit

* split pr

* remove torch script (it gave minor improvemets), delay to pytorch/pytorch#14455 resolution

* fix coadd impl

* coma typo in docs

* nan police float32

* nan police! arcsinh

* typo

* nan police scripted!\nwratpping artanh in a script function results in umstable behavior

* tests

* fix typo

* another test for parallel transport 0

* random doc fix to make typechecker happy

* manifold->module migration fix

* black

* fix test for poincare (autocast double)

* add float32 tests

* fix typo

* rename project->clip tangent

* docs

* fix side effect in tests

* infinity anb beyond test was failing in torch==1.0.1 but not in torch_nightly, acceptable tolerance differs

* add dim argument for poincare math

* batched matvec

* typo in dist formula

* fix tracing issues and grad numerics for Arsinh,Artanh

* _max_norm, specify device + dtype

* clamp before save to backward in artanh

* inplace ops in function impl

* black

* fix typo

* fix spelling

* some fixes to docs

* euclidean -> Euclidean

* black

* math font for number

* random travis fail?

* pytorch future reminder
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.

None yet

5 participants