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

Add Sparse Geodesic Cost Support #677

Merged
merged 26 commits into from
May 20, 2024
Merged

Add Sparse Geodesic Cost Support #677

merged 26 commits into from
May 20, 2024

Conversation

selmanozleyen
Copy link
Collaborator

@selmanozleyen selmanozleyen commented Mar 20, 2024

Here are the steps I planned. I first want to ensure the tests work with this version and I am currently on that step.

  • make sure it works on the new commit of ottjax (here: ott-jax/ott@51a658a)
  • don't densify sparse arrays for geodesic costs
  • adjust and add tests

In order to comply to new version:

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.19%. Comparing base (3151da7) to head (f3c54be).
Report is 25 commits behind head on main.

Current head f3c54be differs from pull request most recent head b054584

Please upload reports for the commit b054584 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #677   +/-   ##
=======================================
  Coverage   78.19%   78.19%           
=======================================
  Files          36       36           
  Lines        3806     3806           
  Branches      706      706           
=======================================
  Hits         2976     2976           
  Misses        524      524           
  Partials      306      306           

see 2 files with indirect coverage changes

@MUCDK
Copy link
Collaborator

MUCDK commented Apr 9, 2024

in the failing tests, set tau_a, tau_b > 0.5

@giovp
Copy link
Member

giovp commented Apr 9, 2024

  • try increase the taus for the failing tests for FGW/GW problems
  • default to None the n iterations

@selmanozleyen
Copy link
Collaborator Author

I tried all of your suggestions and it only worked when I set the tau's to be 1.0 which is balanced. I checked again and it fails after this ott-jax/ott@41906a2 commit specifically. I created a PR separately for the setting the defaults to None #686

@giovp
Copy link
Member

giovp commented Apr 10, 2024

ok then i think we need to raise this to @michalk8 because it is not expected

@MUCDK
Copy link
Collaborator

MUCDK commented Apr 10, 2024

agree, can you maybe try to run the example without moscot, but directly with ott-jax @selmanozleyen ? This might help @michalk8

@selmanozleyen
Copy link
Collaborator Author

agree, can you maybe try to run the example without moscot, but directly with ott-jax @selmanozleyen ? This might help @michalk8

@MUCDK ok I created it here ott-jax/ott#519

@selmanozleyen selmanozleyen mentioned this pull request Apr 30, 2024
4 tasks
@selmanozleyen selmanozleyen requested review from MUCDK and giovp May 7, 2024 18:10
Copy link
Collaborator

@MUCDK MUCDK left a comment

Choose a reason for hiding this comment

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

Can you please also update the tests with geodesic costs for other problems, e.g.

def test_geodesic_cost_set_xy_cost_dense(self, adata_time):

Also, accordingly, please update

Sparse arrays will be always densified.

@MUCDK MUCDK merged commit 1b951fe into main May 20, 2024
7 of 8 checks passed
@MUCDK MUCDK deleted the add/sparse-geodesic branch May 20, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants