Skip to content

[MRG] Regularized OT (optim.cg) bug solve #286

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

Merged
merged 3 commits into from
Sep 28, 2021

Conversation

ncassereau
Copy link
Contributor

@ncassereau ncassereau commented Sep 27, 2021

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Motivation and context / Related issue

Resolves #281, can be reproduced consistently with numpy seed 39.
The test file for da.py sometimes crashes because the alpha variable in optim.cg might be None. This is due to the fact that the derphi variable is close to 0 (i.e. the algorithm has already converged). We solve this by setting alpha to 0, and therefore breaking the loop and returning the computed value.

How has this been tested (if it applies)

Tested on Linux

Checklist

  • The documentation is up-to-date with the changes I made.
  • I have read the CONTRIBUTING document.
  • All tests passed, and additional code has been covered with new tests.

@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #286 (7b2c99a) into master (e0ba31c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #286   +/-   ##
=======================================
  Coverage   93.06%   93.07%           
=======================================
  Files          17       17           
  Lines        3679     3681    +2     
=======================================
+ Hits         3424     3426    +2     
  Misses        255      255           

@rflamary
Copy link
Collaborator

thank you @ncassereau-idris

could you also set a seed for the mapping estimation tests so that the tests are reproducible? and also add a test for the lineseach function?

It was a good thing that it failed because it allowed us to detect it but now we need a new test for this case and fully reproducible tests.

@rflamary rflamary merged commit 7dde9e8 into PythonOT:master Sep 28, 2021
@ncassereau ncassereau deleted the bug_line_search branch October 1, 2021 09:34
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.

Random bug in test_da.py
2 participants