Skip to content

Y padawan/149 refactoring mpsbackendimpl#184

Open
YPadawan wants to merge 15 commits intopasqal-io:mainfrom
YPadawan:YPadawan/149-refactoring-mpsbackendimpl
Open

Y padawan/149 refactoring mpsbackendimpl#184
YPadawan wants to merge 15 commits intopasqal-io:mainfrom
YPadawan:YPadawan/149-refactoring-mpsbackendimpl

Conversation

@YPadawan
Copy link

@YPadawan YPadawan commented Nov 6, 2025

This PR is being opened in order to solve issue #149. Several of the required refactoring has been done. But there are still some doubts and there are still some details to check before eventually merging.

  • MPSBackendImplBase is now an abstract base class where sweep_complete and progress are abstract methods
  • A TDVPBackendImpl has been implemented with the sweep_complete and progress methods that were previously in MPSBackendImplBase. Comments indicated that it was TDVP, but it needs to be confirmed
  • NoisyMPSBackendImpl has become NoisyTDVPBackendImpl, inheriting from TDVPBackendImpl and overriding sweep_complete

In addition to this a new module named permutators.py has been created where all functions making permutations were transferred for clarity purposes. Only backend classes remain in mps_backend_impl.py. Maybe more refactoring should be considered.

  • Linting has been made in mps_backend_impl.py and permutators.py using pylint.

Documentation is still lacking, but can be done before merging.

Tests of test_mps_backend_impl.py are running and all of them pass. But a change has been made in L307 of this file, because the two tensors that were being compared were located in different places, one was in the cpu the second in the gpu. In L307, the relevant tensor is explicitly sent to the cpu (for now). This might be a local problem, or it might have something to do with the test itself. It needs investigating.

When testing all other test files, apart from those that took too long to run almost all of them passed for emu_mps. There was one failure that should be mentioned in here:
test_solver_utils.py::test_evolve_pair
the torch.allclose failed. (It should be out of scope of the issue).

…tter to separate these functions from the mps_backend_impl.py, so that the latter could only contain necessary classes for the effective implementation of the MPS backend.
…hods. It seems that the methods defined in original MPSBackEndImpl were thos of TDVP. But it is not clearly stated. Documentation is lacking
…or2 is explicitly uploaded in the cpu, because it seems to be on the gpu by default. Still need to investigate
@YPadawan
Copy link
Author

YPadawan commented Nov 6, 2025

Of course all comments and constructive criticism is welcome. There are probably some changes to make.

@YPadawan
Copy link
Author

YPadawan commented Nov 6, 2025

Another question, I have opened the PR to merge with the main branch, maybe there is another one where merging usually happens ? Before merging with the main branch ?

Copy link
Contributor

@a-quelle-pasqal a-quelle-pasqal left a comment

Choose a reason for hiding this comment

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

I'm going to keep this open for the time-being, since the work done is good, but I don't think it is finished. The entire point of issue #149 is that the DMRG and TDVP algorithms have significant overlap in terms of their logic. Currently DMRGBackendImpl and TDVPBackendImpl do not share any code in their progress method, and the point was to implement the current class structure, and then share as much of the logic in the respective progress methods in MPSBackendImpl, which requires refactoring mainly of the logic in TDVPBackendImpl.

@YPadawan
Copy link
Author

Ok. I was waiting for an aswer on this. I can keep working on this issue. I can move all common behaviors to the base class. I will make the changes and commit them. Until the code is good for merging.

@YPadawan
Copy link
Author

Hello !
Sorry for the delay, but I was busy lately. I think that I am done with the refactoring of the mps_backend_impl.py. I have have moved bath updates into the base class MPSBackendImpl and both DMRGBackendImpl.progress() and TDVPBackendImpl.progress() call the base class bath update (from left to right and right to left). But the specific logic for each one is still applied naturally. I don't think there a lot more code to share at this level. Let me know if there are still things that I didn't see. The test test_mps_backend_impl.py runs and is all GREEN !

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