-
Notifications
You must be signed in to change notification settings - Fork 506
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
[MRG] Debug convolutional methods that compute barycenters to work with different devices. #533
[MRG] Debug convolutional methods that compute barycenters to work with different devices. #533
Conversation
…re the only ones use linspace)
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #533 +/- ##
==========================================
+ Coverage 96.26% 96.31% +0.05%
==========================================
Files 67 67
Lines 14043 14136 +93
==========================================
+ Hits 13518 13615 +97
+ Misses 525 521 -4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @framunoz for you PR, it looks great. Could you answer my question and comment please?
@@ -352,13 +352,15 @@ def test_sinkhorn2_variants_device_tf(method): | |||
nx.assert_same_dtype_device(Mb, Gb) | |||
nx.assert_same_dtype_device(Mb, lossb) | |||
|
|||
# Check that everything happens on the GPU |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you move this outside of the GPU test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because Codecov does not test these lines, since it works on cpu, giving less coverage to the overall code. When I leave the device verification to the end, Codecov will give coverage to all lines except the last one. Otherwise, it will not give coverage to most of the lines after the if
as it was before.
To summarize, it is rather a trick to increase the coverage, which were cases that can still be included, but due to limitations of the Codecov machine, it gives the impression that it worsens the coverage.
test/test_bregman.py
Outdated
# Using the Tensorflow backend | ||
nx = ot.backend.TensorflowBackend() | ||
|
||
rng = np.random.RandomState(42) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you factorize all this data initialization in a function ad call it each time please (there are other plces where it will be used)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included a function that initializes the matrix A, and replaced it each time it is called.
The extension that reformats the code to comply with the PEP 8 format made many changes to the code. Do I leave it as it was before the commit? Or is the new style preferred? |
Thanks for the changes, I would prefer we stick to the standard from autopep8 ( |
I used black in the tests, although because of the vscode extension, the formatting was applied automatically. In these last commits I left the formatting of the rest of the tests as they were before, also, I added a new line in the |
Types of changes
linspace
method of the backends now has thetype_as
argument to convert to the same dtype and device.convolutional_barycenter2d
andconvolutional_barycenter2d_debiased
functions now work with different devices.Motivation and context / Related issue
The problem that the PR solves is that the
convolutional_barycenter2d
andconvolutional_barycenter2d
functions were not working on different devices (e.g. using PyTorch with CUDA). It was detected that the error came from thelinspace
method of the backends not having atype_as
argument.How has this been tested (if it applies)
test_backend.py
that checks if thetype_as
argument in thelinspace
method works.test_wasserstein_bary_2d_d_dtype_device
,test_wasserstein_bary_2d_device_tf
,test_wasserstein_bary_2d_debiased_dtype_device
andtest_wasserstein_bary_2d_debiased_device_tf
which mirror thetest_sinkhorn2_variants_dtype_device
andtest_sinkhorn2_variants_device_tf
tests to check if the barycenter work with different dtypes and devices.PR checklist