Skip to content

Entropy-regularised Gromov-Wasserstein#165

Open
zsteve wants to merge 27 commits intomasterfrom
gromov
Open

Entropy-regularised Gromov-Wasserstein#165
zsteve wants to merge 27 commits intomasterfrom
gromov

Conversation

@zsteve
Copy link
Copy Markdown
Member

@zsteve zsteve commented Mar 8, 2022

No description provided.

Comment thread src/gromov.jl Outdated
Comment thread src/gromov.jl Outdated
Comment thread src/gromov.jl Outdated
Comment thread test/gromov.jl Outdated
Comment thread test/gromov.jl Outdated
Comment thread test/gromov.jl Outdated
Comment thread test/gpu/simple_gpu.jl Outdated
@zsteve zsteve marked this pull request as ready for review January 23, 2023 04:04
Comment thread .github/workflows/CI.yml Outdated
Comment thread gpu/Manifest.toml Outdated
Comment thread gpu/Project.toml Outdated
Comment thread src/gromov.jl Outdated

function get_new_cost!(C, plan, tmp, Cμ, Cν)
A_batched_mul_B!(tmp, Cμ, plan)
return A_batched_mul_B!(C, tmp, -4Cν)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The multiplication with -4 introduces additional allocations. I wonder if this could be avoided, e.g., by updating in-place or some additional cache.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dealt with now (in-place scaling of tmp)

Comment thread src/gromov.jl Outdated
if to_check_step == 0 || iter == maxiter
# reset counter
to_check_step = check_convergence
isconverged = sum(abs, plan - plan_prev) < max(_atol, _rtol * norm_plan)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

norm_plan is never updated it seems but always set to sum(plan) of the initial randomly initialized plan?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also avoid allocations here by writing:

Suggested change
isconverged = sum(abs, plan - plan_prev) < max(_atol, _rtol * norm_plan)
plan_prev .-= plan # used as a temporary array here to reduce allocations
isconverged = sum(abs, plan_prev) < max(_atol, _rtol * norm_plan)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

norm_plan is never updated it seems but always set to sum(plan) of the initial randomly initialized plan?

The initial plan is taken to be the independent coupling and here we only consider the balanced problem, so norm_plan should not change. I agree however this is a special case of the unbalanced problem where it would not be constant.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also avoid allocations here by writing:

Good catch, done

Comment thread test/Project.toml Outdated
Comment thread test/runtests.jl Outdated
Comment thread test/runtests.jl Outdated
zsteve and others added 3 commits January 27, 2023 18:37
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
@coveralls
Copy link
Copy Markdown

coveralls commented Jan 27, 2023

Pull Request Test Coverage Report for Build 4024858001

Details

  • 33 of 33 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 95.518%

Totals Coverage Status
Change from base Build 2897651829: 0.2%
Covered Lines: 682
Relevant Lines: 714

💛 - Coveralls

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 27, 2023

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.53%. Comparing base (9da044c) to head (39f0b36).
⚠️ Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
+ Coverage   95.30%   95.53%   +0.22%     
==========================================
  Files          14       15       +1     
  Lines         681      716      +35     
==========================================
+ Hits          649      684      +35     
  Misses         32       32              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/gromov.jl Outdated
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.

4 participants