-
Notifications
You must be signed in to change notification settings - Fork 0
Fix canonize
function and enhance index semantics
#8
Conversation
Mmmm... This did not fail on my machine... |
I think it fails because some of this features require |
Ahh, it has to be that. Yes, that would be great, @mofeing ! |
src/Ansatz/Chain.jl
Outdated
rightsite(::Union{Open, Periodic}, tn::Chain, site::Site) = Site(site.id + 1) | ||
|
||
leftsite(tn::Chain, site::Site) = leftsite(boundary(tn), tn, site) | ||
leftsite(::Open, tn::Chain, site::Site) = Site(site.id - 1) | ||
leftsite(::Union{Open, Periodic}, tn::Chain, site::Site) = Site(site.id - 1) |
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.
Can we canonize a periodic MPS? If so, doing a ::Union{Open,Periodic}
is probably not the best solution.
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.
We can, but we need to include the proper logic on the canonize
function. Nevertheless, we can always do leftsite
and rightsite
of Periodic
MPS
.
However, maybe it would be a good idea to put here a check so we don't go to the left of the Site(1)
, in the case of Open
MPS
.
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.
I think it would be better to do 2 separa methods:
- One, the
leftsite(::Open)
as it is now - The other
leftsite(::Periodic)
that wraps thei
with the period
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.
Done, what do you think?
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.
Much better, but we should check whether site
is in range for the Periodic
case.
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.
This is not necessary, right? In Periodic
we can assume Site(4)
is effectively Site(1)
, for a periodic cell of 3
sites.
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.
We can do that, but we have to stick to it then.
Summary
This PR fixes the
canonize
function, enabling it to correctly canonize a desiredTensor
within aSite
of aChain
. We have enhanced this function to allow users to choose themode
(:qr
or:svd
) in which the canonization is applied. By default, the:qr
option is selected, preserving the total number of tensors in theTensorNetwork
. The:svd
mode, on the other hand, introduces a new tensor which is connected by a hyperindex to the bond between sites, and it contains the Schmidt values. Additionally, we have created a comprehensive test set to ensure the function's reliability and performance.Furthermore, we have refactored the logic inside
leftindex
andrightindex
. Now, these indices are determined by the intersection of the indices with those of the adjacent tensor in theChain
, rather than relying on the order ofinds
in the tensors. This enhancement is expected to make our library more robust and error-resistant, as changes in the tensor order will no longer have unintended side effects.Example
Here we show how we create a
Chain
and we canonize the middle tensor: