QuantumKitHub / MPSKit.jl

A Julia package dedicated to simulating quantum many-body systems using Matrix Product States (MPS)
MIT License
139 stars 30 forks source link

3 site tests #180

Closed Gertian closed 3 weeks ago

Gertian commented 1 month ago

This PR adds tests for infinite MPS with a 3-site unit cell.

I ran this locally and found an almost unexciting additional cost to run these on my machine.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

see 3 files with indirect coverage changes

Gertian commented 1 month ago

I think this is a great addition. My only comment on this one is that maybe instead of an additional testset which is separate, you could merge this in each of the testsets for the ground state algorithms above, which would then consist of:

  • test logging (1site)
  • test algorithm (1site)
  • repeat to 3sites
  • test logging (3 sites)
  • test algorithm (3 sites)

This would bundle things by algorithm, so for example if VUMPS contains an error it will fail sooner, and also reduce runtime even more because you start from something near converged.

(This is mostly just my preference/opinion, if you disagree I would be fine merging as-is)

Ps, I would probably merge this first, which would then resolve the codecov complaints of the other PRs

Sure, I'll change this tomorrow :)

Gertian commented 1 month ago

All rightly @lkdvos, this is all done now :)

VictorVanthilt commented 3 weeks ago

I'm okay with adding this before the blocktensor refactor. If the tests pass here it's a good extra testset for the blocktensor2 branch to pass.

Gertian commented 3 weeks ago

@lkdvos as far as I understand the tests also don't consider ''parallelize_sites = true'' and ''false'' etc.

I agree that a user triggered test should obly trigger one config. But when PR are tested all permutations of the compilation flags should be considered no ?

lkdvos commented 3 weeks ago

Yeah, I agree there. What I had in mind was to change the entire current approach to work with OhMyThreads.jl instead, such that we can have schedulers instead of the Preferences.jl based mechanism, to allow for dynamic changing of the scheduler. The problem is that I wanted to put this off until blocktensor gets merged, and did not have too much time lately to really get into that. The idea is that the current implementation is quite unwieldy, and has too much code duplication, and it is really easy to circumvent that, but I havent gotten around to finding the time. I also don't want to halt progress, which is why I am trying to merge these things now before the blocktensor changes, as that still depends on TensorKit which is not yet finished.

Gertian commented 3 weeks ago

Yeah, I agree there. What I had in mind was to change the entire current approach to work with OhMyThreads.jl instead, such that we can have schedulers instead of the Preferences.jl based mechanism, to allow for dynamic changing of the scheduler. The problem is that I wanted to put this off until blocktensor gets merged, and did not have too much time lately to really get into that. The idea is that the current implementation is quite unwieldy, and has too much code duplication, and it is really easy to circumvent that, but I havent gotten around to finding the time. I also don't want to halt progress, which is why I am trying to merge these things now before the blocktensor changes, as that still depends on TensorKit which is not yet finished.

Ok great. I think its very reasonable to put this off for now. I just wanted to make sure that you were aware of this.

In the future I'd be happy to help implementing this together with the other parallelizations that I made :)