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

Combine pr #182

Closed
wants to merge 14 commits into from
Closed

Combine pr #182

wants to merge 14 commits into from

Conversation

Gertian
Copy link
Collaborator

@Gertian Gertian commented Oct 15, 2024

In case it's more convenient for the main devs (I guess @lkdvos ), this PR merges my three prior PRs into one.

So in this PR I've combined

  1. parallel GD
  2. parallel VumpsSvd
  3. tests for n-site unit cells

Most notably the new tests seemed important for me since the parallel GD and VumpsSvd might only fail when multiple sites are present and the parallel code kicks in...

At the very least this PR allows the reviewer to see that the new code works with the new tests :)

It's equivalent to merge this PR or merge the 3 previous PRs that I just did :)
Like I said, whatever is most convenient...

@lkdvos
Copy link
Member

lkdvos commented Oct 15, 2024

I'm gonna do my best to get to all of this and look at everything as soon as possible, I'm a bit short on time these days. (Moving houses, traveling and preparing presentations seem to be taken up a lot of time...)
As a small side-note, I wanted to ask how urgent this is. I do agree we want to add is, but we have an upcoming major rewrite in the branch blocktensor2, which is more or less working but requires an updated version of tensorkit first, which can then release blocktensorkit, which then makes that pr mergable. I am a bit hesitant to make many intermediate changes to avoid having to port over too much of the code, but I also see that it is not reasonable to completely stop development in the meantime, so it's all about striking a balance.
Anyways, if I still didn't get to this by end of next week, feel free to ping me again!

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 84.21053% with 12 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/algorithms/grassmann.jl 76.19% 10 Missing ⚠️
src/algorithms/changebonds/vumpssvd.jl 94.11% 2 Missing ⚠️
Files with missing lines Coverage Δ
src/algorithms/changebonds/vumpssvd.jl 97.22% <94.11%> (+25.42%) ⬆️
src/algorithms/grassmann.jl 92.95% <76.19%> (-7.05%) ⬇️

... and 4 files with indirect coverage changes

@Gertian
Copy link
Collaborator Author

Gertian commented Oct 15, 2024

Hi,

Thanks for the quick reply.

I wouldn't say its super urgent, but on the other hand, for my usecase with unit cell 32 this should amount to a HUGE speed increase.

I guess I can always use my local branch though. I don't like doing this though.

Ps : a lot of the commits are me forgetting to format. Or fixing typos. The actual amount of new code is quite minimal.

@Gertian
Copy link
Collaborator Author

Gertian commented Oct 15, 2024

@lkdvos seems like you've already reviewed the other PRs so this is irrelevant now.

As you suggested the best way forward is probably to first merge the tests and then the other two PRs !

@Gertian Gertian closed this Oct 15, 2024
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