-
Notifications
You must be signed in to change notification settings - Fork 26
Reorganize CTMRG contraction code #326
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
Conversation
|
I still have some reordering to push, but I'll wait until the current test finishes. |
|
Bad news: we can't just merge this PR due to the poor test coverage (50%). It appears to be mainly due to not testing CTMRG for PEPS-PEPO-PEPS sandwich. The very low coverage of @pbrehmer We may need your help to increase test coverage, or identify and remove things that are not used. |
|
The low test coverage of So at least with respect to the half/full-infinite environment things, the low test coverage isn't saying much. We could easily increase it by just commenting out the unused parts. I could do that if we want to do it. (Not sure about the PEPS-PEPO-PEPS things though, I assumed there was tests for that...) |
lkdvos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, I would suggest to not do any of that in this PR, I really like that this is only reorganizing the files.
Overall I think this is a great change, let's wait for the tests to run just to be sure but since we aren't actually changing any of the test coverage I would just merge afterwards.
This PR splits
src/algorithms/contractions/ctmrg_contractions.jlinto several shorter files in the foldersrc/algorithms/contractions/ctmrg/for easier navigation among different parts.No changes are made in docstrings or code body, so the PR can be safely merged as long as the tests pass.