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

Parallel vumpssvd #181

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Gertian
Copy link
Collaborator

@Gertian Gertian commented Oct 15, 2024

This PR also makes vumpSVD parallel.

Again tests have been implemented in another PR and I've tested the combined PR locally on my machine :)

@Gertian Gertian requested a review from lkdvos October 15, 2024 20:54
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/algorithms/changebonds/vumpssvd.jl 88.23% 4 Missing ⚠️
Files with missing lines Coverage Δ
src/algorithms/changebonds/vumpssvd.jl 79.16% <88.23%> (+7.37%) ⬆️

... and 2 files with indirect coverage changes

Copy link
Member

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

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

Is this ever a bottleneck? Because this definitely nontrivially changes the algorithm by doing it in parallel, because every bond update is assuming the other sites to be constant, and this implementation breaks that assumption. It might indeed be the case that this does not matter, you're interested in getting the spaces approximately right, but I'm somewhat less confident about this

@Gertian
Copy link
Collaborator Author

Gertian commented Oct 15, 2024

Is this ever a bottleneck? Because this definitely nontrivially changes the algorithm by doing it in parallel, because every bond update is assuming the other sites to be constant, and this implementation breaks that assumption. It might indeed be the case that this does not matter, you're interested in getting the spaces approximately right, but I'm somewhat less confident about this

I just had some code running where this took about half a day. And bond dimensions are still growing.

I wouldn't say its the biggest bottleneck but I was parallelizing stuff anyways so I thought, why not...

I agree that this changes the algorithm, hence my choice to explicitly retain the old algorithm in the non parallel case.
I don't think the change is bad though, parallel VUMPS also breaks this assumption and still converges eventually. So I don't see any reason why this wouldn't give something reasonable.

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.

2 participants