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

bug fix: avoid mixing up linear and quadratic in genot #517

Merged
merged 5 commits into from
Apr 12, 2024

Conversation

soerenab
Copy link
Contributor

@soerenab soerenab commented Apr 9, 2024

This fixes #514.

prepare_data() now returns a dict with the right keys for the linear or quadratic matching function.

@michalk8 michalk8 added the bug Something isn't working label Apr 9, 2024
@michalk8
Copy link
Collaborator

michalk8 commented Apr 9, 2024

@soerenab could you please fix the failing tests?

2024-04-09T14:13:11.3993310Z FAILED tests/neural/methods/genot_test.py::TestGENOT::test_genot[lin_dl] - TypeError: data_match_fn() got an unexpected keyword argument 'x'
2024-04-09T14:13:11.3995788Z FAILED tests/neural/methods/genot_test.py::TestGENOT::test_genot[quad_dl] - TypeError: data_match_fn() got an unexpected keyword argument 'xx'
2024-04-09T14:13:11.3998067Z FAILED tests/neural/methods/genot_test.py::TestGENOT::test_genot[fused_dl] - TypeError: data_match_fn() got an unexpected keyword argument 'x'
2024-04-09T14:13:11.4000374Z FAILED tests/neural/methods/genot_test.py::TestGENOT::test_genot[lin_cond_dl] - TypeError: data_match_fn() got an unexpected keyword argument 'x'
2024-04-09T14:13:11.4002702Z FAILED tests/neural/methods/genot_test.py::TestGENOT::test_genot[quad_cond_dl] - TypeError: data_match_fn() got an unexpected keyword argument 'xx'
2024-04-09T14:13:11.4005049Z FAILED tests/neural/methods/genot_test.py::TestGENOT::test_genot[fused_cond_dl] - TypeError: data_match_fn() got an unexpected keyword argument 'x'

@michalk8
Copy link
Collaborator

michalk8 commented Apr 9, 2024

Afterwards, LGTM and can merge, the failing
2024-04-09T14:13:12.4681618Z FAILED tests/tools/soft_sort_test.py::TestSoftSort::test_soft_sort_jacobian[True] - jaxlib.xla_extension.XlaRuntimeError: INTERNAL: Generated function failed: CpuCallback error: Traceback (most recent call last): is unrelated and I need to reproduce locally.

@soerenab
Copy link
Contributor Author

The above mentioned tests now pass. Overall, this PR makes the following changes:

  • prepare_data() in GENOT no longer returns src_quad and tgt_quad if these are None (as the linear matching function does not take them as arguments)
  • the order of arguments in match_quadratic() has changed from xx, yy, x, y to x, y, xx, yy to be consistent with match_linear()
  • In genot I replaced data_match_fn() with get_match_function(). The previous combination of Optional arguments and functools.partial does not seem to work -> I always got errors of missing args in the linear case even though those args were not needed and indeed optional. This new implementation is also cleaner in my opinion.

Copy link

codecov bot commented Apr 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.92%. Comparing base (4bba69f) to head (42b05a9).
Report is 3 commits behind head on main.

❗ Current head 42b05a9 differs from pull request most recent head 2bbf904. Consider uploading reports for the commit 2bbf904 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
+ Coverage   90.62%   90.92%   +0.30%     
==========================================
  Files          68       68              
  Lines        7047     7044       -3     
  Branches      685      998     +313     
==========================================
+ Hits         6386     6405      +19     
+ Misses        501      485      -16     
+ Partials      160      154       -6     
Files Coverage Δ
src/ott/neural/methods/flows/genot.py 84.61% <100.00%> (+0.13%) ⬆️
src/ott/solvers/utils.py 90.69% <ø> (ø)

... and 1 file with indirect coverage changes

@michalk8 michalk8 merged commit 458e265 into ott-jax:main Apr 12, 2024
12 checks passed
michalk8 added a commit that referenced this pull request Jun 27, 2024
* bug fix: avoid mixing up linear and quadratic part by returning Dict in genot prepare_data()

* fix data_match_fn() setup in genot tests

* prepare_data() in GENOT now returns a tuple instead of a dict; change order of args in utils.match_quadratic()

* Update docs

* Fix typo

---------

Co-authored-by: Michal Klein <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

linear and quadratic part get mixed up in genot
2 participants