-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Conversation
Thanks! @ferrine This is very insightful! |
Very interesting, perhaps we can add more models (Lorentz hyperboloid model which is more stable for numerical optimization) later? |
@KhrulkovV Hi! Happy to see you there. I'll check that paper! |
I'm done. There are some docs+implementation. Waiting for tests pass now |
Tests with adam do not work if I use torch script with nontensor arguments, as: |
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, |
Sure, I have docs with plots also. Here is the requested source code
https://github.com/ferrine/geoopt/blob/f7e842d559fc6e9de9b02bec31420ec1888df6b8/docs/plots/extended/poincare/parallel_transport.py
And docs also
https://geoopt.readthedocs.io/en/poincare/extended/poincare.html
Docs will change soon. I think they worth to be polished
…--
Max
вт, 26 февр. 2019 г., 0:51 Qi Liu <notifications@github.com>:
Hi @ferrine <https://github.com/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
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ALKb7hZwlhRD-pN_8CGj8KjQ9uzlyzd6ks5vRFrngaJpZM4a6dK4>
.
|
Thanks! @ferrine This is very helpful. |
6ce5ed1
to
6025b5e
Compare
Could anyone of you review docs here? Maybe @leuchine or @newkozlukov if you have free time? The checklist is the following:
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. |
Thanks! @ferrine I will try to read it this week. |
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 |
There is a small typo in docs: distance on hyperboloid is arccosh(x, y), not arccos(x, y). Will read more carefully later. |
I used a capital letter to distinguish this operation from regular The code from FB is great. I should take their best practice implementations using a |
@ferrine It is not a big problem. Just a small convention in math publications I think. :) Low priority. Thanks! |
Any additional comments, guys? Eager to continue the PR |
I've observed weird behavior in |
There was a problem hiding this 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.
There are some fixes I have to do in docs...
…On Fri, Mar 22, 2019, 10:10 Rasul Kerimov ***@***.*** wrote:
***@***.**** approved this pull request.
I'm approving it for now with some uncertainty. More analysis is required
for this branch.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALKb7vZESWBDHDwhq3gNU0CcIjifsJ_cks5vZIHhgaJpZM4a6dK4>
.
|
@leuchine, @KhrulkovV going to finish this in 5hrs today. If you have any comments, this is the best time to write them. |
todos for today:
|
I faced numerical issues implementing #59, I have concerns about this change |
@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. |
There was a problem hiding this 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.
Thank you all! |
* 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
Huray, we are ready to start.
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):
1 − eps
, whereeps = 10^{-5}
eps = 10^{-15}
[-15, 15]
input totanh
and cliptanh^{-1}
to[-1+eps, 1-eps]
CC @leuchine!