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

[MRG] Gromov_Wasserstein2 not performing backward properly on GPU #352

Merged
merged 2 commits into from
Mar 2, 2022

Conversation

ncassereau
Copy link
Contributor

@ncassereau ncassereau commented Mar 2, 2022

Types of changes

The backpropagation is not working in gromov_wasserstein2 if the given tensors are located on a GPU. This is due to the fact that part of the computation is performed with numpy and the device was forgotten when casting back to torch.

Motivation and context / Related issue

Resolves #351

How has this been tested (if it applies)

PR checklist

  • I have read the CONTRIBUTING document.
  • The documentation is up-to-date with the changes I made (check build artifacts).
  • All tests passed, and additional code has been covered with new tests.
  • I have added the PR and Issue fix to the RELEASES.md file.

@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #352 (2d045de) into master (1781472) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #352      +/-   ##
==========================================
- Coverage   93.63%   93.60%   -0.04%     
==========================================
  Files          22       22              
  Lines        5591     5595       +4     
==========================================
+ Hits         5235     5237       +2     
- Misses        356      358       +2     

@tbng
Copy link
Contributor

tbng commented Mar 2, 2022

Thanks for the quick PR! One thing I wonder though is that how to check if the build on Circle CI does run the test with GPU? https://github.com/PythonOT/POT/runs/5389073677?check_suite_focus=true

Because if you do this on the test file POT/test/testgromov.py:

https://github.com/ncassereau-idris/POT/blob/c0b2eda0ccdd6da7e7f5296c9abf4e0e9720ec0e/test/test_gromov.py#L185-L186

if torch.cuda.is_available():
    devices.append(torch.device("cuda"))

This line might not be hit at all during running the test file

@ncassereau
Copy link
Contributor Author

ncassereau commented Mar 2, 2022

No you're right, tests performed here do not have access to a GPU. But I ran it on my machine which is equipped with V100s and the test passes. As always with GPU tests, they should be run locally to detect any issue.

@tbng
Copy link
Contributor

tbng commented Mar 2, 2022

No you're right, tests performed here do not have access to a GPU. But I ran it on my machine which is equipped with a V100 and the test passes. As always with GPU tests, they should be run locally to be detected.

The tests passed on my local run as well, thanks.

@ncassereau ncassereau changed the title [WIP] Gromov_Wasserstein2 not performing backward properly on GPU [MRG] Gromov_Wasserstein2 not performing backward properly on GPU Mar 2, 2022
@ncassereau ncassereau marked this pull request as ready for review March 2, 2022 10:20
@rflamary
Copy link
Collaborator

rflamary commented Mar 2, 2022

Thank you to both of you this was a quickly found and squashed bug, we need to do a release soon

@rflamary rflamary merged commit 9412f0a into PythonOT:master Mar 2, 2022
@ncassereau ncassereau deleted the gromov_wasserstein2_backward branch March 2, 2022 11:46
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.

ot.gromov.gromov_wasserstein2 loss does not perform backprop with torch CUDA tensor
3 participants