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

Feature/update prox #575

Merged
merged 19 commits into from
Sep 3, 2024
Merged

Feature/update prox #575

merged 19 commits into from
Sep 3, 2024

Conversation

michalk8
Copy link
Collaborator

closes #574

@michalk8 michalk8 added the enhancement New feature or request label Aug 23, 2024
@michalk8 michalk8 self-assigned this Aug 23, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.78%. Comparing base (ea7d5b9) to head (3c41171).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #575      +/-   ##
==========================================
+ Coverage   87.76%   87.78%   +0.01%     
==========================================
  Files          72       72              
  Lines        7791     7801      +10     
  Branches     1124     1126       +2     
==========================================
+ Hits         6838     6848      +10     
  Misses        802      802              
  Partials      151      151              
Files with missing lines Coverage Δ
src/ott/geometry/costs.py 97.22% <100.00%> (+0.05%) ⬆️
src/ott/geometry/regularizers.py 98.65% <100.00%> (+0.01%) ⬆️

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!! mostly minor comments.

src/ott/geometry/costs.py Outdated Show resolved Hide resolved
src/ott/geometry/regularizers.py Outdated Show resolved Hide resolved
src/ott/geometry/regularizers.py Outdated Show resolved Hide resolved
src/ott/geometry/regularizers.py Outdated Show resolved Hide resolved
tests/geometry/costs_test.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pkassraie pkassraie left a comment

Choose a reason for hiding this comment

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

This PR effectively moves the regularization parameter $\lambda$ out of the signature for the regularization function, and attributes it to the overall loss, which I think makes more sense. This is done by adding the PostComposition class and updating the signatures. All good with me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a pointer that the function definition of the overlap norm is the mathematical definition of the overlap norm divided by 2 (see line 418 of regularizers.py).

src/ott/geometry/costs.py Outdated Show resolved Hide resolved
src/ott/geometry/costs.py Outdated Show resolved Hide resolved
@michalk8 michalk8 merged commit 4aed3ec into main Sep 3, 2024
12 checks passed
@michalk8 michalk8 deleted the feature/update-prox branch September 3, 2024 20:01
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.

Feature request: Adding lam to the regularizer class
3 participants