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

Remove functional API #222

Merged
merged 21 commits into from
Jan 7, 2023
Merged

Conversation

michalk8
Copy link
Collaborator

@michalk8 michalk8 commented Jan 4, 2023

In this PR:

  • remove internal make functions
  • remove ott.tools.transport interface (was used sparsely); should be re-introduced in the future when creating abstraction for solver outputs
  • rename sinkhorn.sinkhorn -> sinkhorn.solve
  • rename gromov_wasserstein.gromov_wasserstein -> gromov_wasserstein.solve
  • polish docs
  • udpate notebooks to use the problems/solver classes
  • better isort (also enabled for notebooks)
  • update some tests (e.g., Sinkhorn initializers)

TODOs:

  • update tutorials that reference the removed functions

@michalk8 michalk8 self-assigned this Jan 4, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2023

Codecov Report

Merging #222 (ce59e8e) into main (a6feea4) will decrease coverage by 8.75%.
The diff coverage is 69.09%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #222      +/-   ##
==========================================
- Coverage   89.21%   80.46%   -8.76%     
==========================================
  Files          52       52              
  Lines        5315     5299      -16     
  Branches      549      542       -7     
==========================================
- Hits         4742     4264     -478     
- Misses        438      886     +448     
- Partials      135      149      +14     
Impacted Files Coverage Δ
src/ott/initializers/linear/initializers.py 92.63% <ø> (-5.27%) ⬇️
src/ott/problems/linear/linear_problem.py 100.00% <ø> (ø)
src/ott/problems/quadratic/quadratic_problem.py 88.26% <ø> (-1.12%) ⬇️
src/ott/solvers/linear/acceleration.py 96.61% <ø> (ø)
src/ott/solvers/linear/sinkhorn_lr.py 95.33% <ø> (-0.88%) ⬇️
src/ott/tools/segment_sinkhorn.py 40.00% <20.00%> (-60.00%) ⬇️
src/ott/tools/plot.py 19.81% <47.61%> (+1.89%) ⬆️
src/ott/solvers/quadratic/gromov_wasserstein.py 81.67% <75.00%> (-7.91%) ⬇️
src/ott/tools/sinkhorn_divergence.py 63.15% <80.00%> (-35.09%) ⬇️
src/ott/solvers/linear/sinkhorn.py 96.75% <100.00%> (-0.18%) ⬇️
... and 36 more

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@michalk8 michalk8 marked this pull request as ready for review January 5, 2023 17:56
@michalk8 michalk8 mentioned this pull request Jan 5, 2023
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 a lot Michal, this is indeed cleaner now.

from ott.solvers.linear import sinkhorn


def run_sinkhorn(
Copy link
Contributor

Choose a reason for hiding this comment

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

this feels copy pasted from sinkhorn_divergence.

is there a place this could be stored that's more visible? or maybe stored in sinkhorn.py?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added as sinkhorn.solve + this function now also supports rank, to make it consistent with gromov_wasserstein.solve.

@michalk8 michalk8 added the enhancement New feature or request label Jan 6, 2023
@michalk8 michalk8 added deprecation documentation Improvements or additions to documentation labels Jan 6, 2023
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 a lot for this massive refactoring, this all looks a lot cleaner! LGTM.

@michalk8 michalk8 merged commit 7b14b52 into ott-jax:main Jan 7, 2023
@michalk8 michalk8 deleted the fix/remove-functional-api branch January 7, 2023 11:02
michalk8 added a commit that referenced this pull request Jun 27, 2024
* Remove `sinkhorn function`

* Fix `sinkhorn_divergence` test

* Remove `gromov_wasserstein` function

* Remove `make` functions

* Fix `soft_sort` and Jacobian tests

* Remove `Transport` interface

* Fix Jacobian test

* Fix `soft_sort` and tests

* Clean up some tests

* Fix wrong `value_and_grad` usage

* Update notebooks, isort and pre-commit

* [ci skip] Fix rendering in `Sinkhorn`

* Handle TODOs, clean initializer tests

* Add `sinkhorn.solve` utility

* Re-add `gromov_wasserstein.solve`, polish docs

* Remove redundant line from `pyproject.toml`

* Polish quad docs

* Add rank to `sinkhorn.solve`

* Add `rank` to `sinkhorn.solve`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants