-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
A bit better public api #40
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 tasks
This PR helps with #33 |
andbloch
pushed a commit
to andbloch/geoopt
that referenced
this pull request
Dec 29, 2019
* propose public api * add description to docs, ferify tests * docs+black\nmove notes section below return section * set adequate defaults for t * black * order note * refactor developer api * black * add more docs * remove unnessesary code from Euclidean * set order refactor * fix error message * create developer section in docs * create developer section in docs * improve docs * a bit more consistent pytorch usage * black * blank line in the end of the file
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
API change
The purpose of this PR is to allow users to use the library in general setting. The current API does not provide enough flexibility in terms of such kind of operations:
x
toy
Manifold.retr
These simple features require workarounds in development if we choose the way to maintain the very early API. Therefore I propose to change methods signatures a bit (the change in possible old code is mechanical) and introduce new optional arguments.
Breaking changes
Retraction
Vector transport
Retraction + vector transport
order
is an order of retraction approximation, by default uses the simplest that is usually a first order approximation. Possible choices depend on a concrete manifold and -1 stays for exponential mapexponential map is assumed to be a separate method
Migration
Vector transport
Retraction + vector transport
New methods
Some new methods are introduced in the base
Manifold
class:This method returns best possible approximation (of maximum order) for retraction map. Exponential map is assumed to be ideal and has order
-1
.Same holds for exponential map + vector transport
It will be possible to specify the default order on per manifold basis with the following method
Developer API changes
Base class
From now I propose to use a metaclass that tracks and registers retractions. Right after a class creation, it filters
dir(cls)
and looks for special declared methods. If a method is not implemented, then it should begeoopt.base.not_implemented
.geoopt.base.not_implemented
is just a placeholder for a function that raises not implemented error.Special private functions contain the following:
r"^_retr(\d+)?$"
for retraction of a given order (if int postfix provided)r"^_retr(\d+)?_transp$"
for retraction and transportr"^_expmap$"
for exponential map (retraction with order-1
)r"^_expmap_transp$"
for exponential map + vector transport (retraction with order-1
)r"^_transp_follow(\d+)?$"
vector transport that uses direction + retraction rather that the final pointr"^_transp_follow_expmap$"
vector transport that uses direction + exponential map rather that the final point (retraction+transport with order-1
)After this all is registered in
MethodDict
(default dict withnot_implemented
as missing value)With this dict it comes possible to define generic dispatch methods for different orders of approximations like this:
As you see, we avoid weird code that makes use of
if
orgetattr
with handling exceptions.Exponential map is dispatched in the same way
To make
retr_transp
work in case of_transp2y
is much more efficient than_transp_follow
there is a class attribute_retr_transp_default_preference
to indicate this. The attribute should be present in the class definition if differs from default provided inManifold
Deprecations
methods like
are deprecated in favour of their unified alternatives
transp2y
method appears to be very useful sometimes and allows public interface to have optionaly
intransp