-
Notifications
You must be signed in to change notification settings - Fork 483
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
[Fix] Gradient scaling in Partial GW solver #602
base: master
Are you sure you want to change the base?
Conversation
Hello @yikun-baio and thanks for the PR. You should not propose anew file but instead implement directly your fix in th existing files. This is important because we have many tests that checks that nothing else breaks and we can compare easily your modification with the old implementation. We will do a code review with @lchapel when this is done. |
Hello @rflamary, Thank you for your feedback. I apologize for not implementing the fix directly in the existing file and instead proposing a new file. This is my first time to contribute to a public project via a pull request. I just realized that my email settings were inadvertently blocking emails from GitHub, which caused me to delay seeing your message. Not sure if it's still needed, but I'll implement the fix as suggested and make sure it's done in an existing file for @lchapel's direct code review. Thank you very much for your guidance and I look forward to contributing more effectively in the future. Thank you for your understanding and patience. Sincerest regards, |
Hello @yikun-baio this is a friendly reminder to implement your fix directly in the code if possible. I used a rocket emoji in your previous question to say that we are interested but maybe you did not receive a notfication. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #602 +/- ##
==========================================
- Coverage 96.77% 96.71% -0.07%
==========================================
Files 81 82 +1
Lines 16106 16173 +67
==========================================
+ Hits 15587 15642 +55
- Misses 519 531 +12 |
Types of changes
I modify the code ot.partial.partial_gromov_wasserstein
Motivation and context / Related issue
There seems to be an inconsistency between ot.partial.partial_gromov_wasserstein and the line search section in the paper [29]. I fixed this section. In addition, I have made minor change to the initial guess in the partial-GW solver since the original initial guess is np.out(p,q), which might not be suitable for unbalanced case, i.e. |p|\neq |q|.
N/A
N/A
How has this been tested (if it applies)
PR checklist