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

Meta OT Initializer #145

Merged
merged 11 commits into from Oct 12, 2022
Merged

Meta OT Initializer #145

merged 11 commits into from Oct 12, 2022

Conversation

bamos
Copy link
Contributor

@bamos bamos commented Oct 10, 2022

This is an initial version of a Meta OT initializer along with a notebook demonstrating it's usage.
Here is how the notebook renders in the website, and here are how the code docs render:

image
image
image

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@bamos bamos changed the title Meta OT Initializers Meta OT Initializer Oct 10, 2022
@michalk8 michalk8 added the enhancement New feature or request label Oct 10, 2022
Copy link
Contributor

@marcocuturi marcocuturi left a comment

Choose a reason for hiding this comment

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

Thanks Brandon!! this looks fantastic as an addition!! a few minor comments + suggestion to use ent_reg_cost directly (open question would be to handle tau_... with MetaOT... Maybe for now set to 1...)

ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2022

Codecov Report

Merging #145 (d27731a) into main (8a8e406) will increase coverage by 0.04%.
The diff coverage is 93.05%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #145      +/-   ##
==========================================
+ Coverage   89.80%   89.84%   +0.04%     
==========================================
  Files          51       51              
  Lines        4971     5040      +69     
  Branches      505      509       +4     
==========================================
+ Hits         4464     4528      +64     
- Misses        386      389       +3     
- Partials      121      123       +2     
Impacted Files Coverage Δ
ott/core/initializers.py 96.93% <93.05%> (-3.07%) ⬇️

@bamos
Copy link
Contributor Author

bamos commented Oct 10, 2022

@marcocuturi - Thanks, I just pulled the latest main, pushed in updates, responded inline/resolved the inline comments above, and re-ran the notebook with the latest version of the code

Copy link
Contributor

@marcocuturi marcocuturi left a comment

Choose a reason for hiding this comment

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

This looks good for me!! @michalk8 want to take a look?

@michalk8 michalk8 self-requested a review October 10, 2022 21:40
Copy link
Collaborator

@michalk8 michalk8 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @bamos , I have made a few comments, will look at the notebook later!

ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
@bamos bamos requested a review from michalk8 October 11, 2022 15:23
@bamos
Copy link
Contributor Author

bamos commented Oct 11, 2022

Thanks @michalk8! I just pushed in most of your suggestions in bb34644, resolved them inline above, and requested a re-review. I've kept the last two outstanding ones open on the naming and geom.dtype. I'll re-run the notebook and push the new output there once these are resolved.

Copy link
Collaborator

@michalk8 michalk8 left a comment

Choose a reason for hiding this comment

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

Thanks @bamos , just minor changes needed.

ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
ott/core/initializers.py Outdated Show resolved Hide resolved
docs/notebooks/MetaOT.ipynb Show resolved Hide resolved
docs/notebooks/MetaOT.ipynb Show resolved Hide resolved
docs/notebooks/MetaOT.ipynb Show resolved Hide resolved
docs/notebooks/MetaOT.ipynb Show resolved Hide resolved
docs/notebooks/MetaOT.ipynb Show resolved Hide resolved
docs/notebooks/MetaOT.ipynb Show resolved Hide resolved
docs/notebooks/MetaOT.ipynb Show resolved Hide resolved
@bamos
Copy link
Contributor Author

bamos commented Oct 12, 2022

I've updated the name to MetaInitializer and have addressed/responded to the other comments inline above. Should I also squash the commits here before merging/rebasing this in?

@michalk8
Copy link
Collaborator

Should I also squash the commits here before merging/rebasing this in?

There's no need to do this on your side, we do by default squash merges on PRs.

LGTM, thanks @bamos, I think this is a great addition!

@michalk8 michalk8 merged commit 9d76c04 into ott-jax:main Oct 12, 2022
@bamos
Copy link
Contributor Author

bamos commented Oct 12, 2022

Thanks for the help and for going through everything so thoroughly! :-)

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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants