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.

Sign up for GitHub

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

Improve compatibility with tensor networks without internal vertices #144

Closed
wants to merge 3 commits into from
Closed

Improve compatibility with tensor networks without internal vertices #144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a763414
Make internal vertices without siteindices compare equal in `hascommo…
b-kloss Mar 6, 2024
e3d4793
Add test.
b-kloss Mar 6, 2024
576bed3
Add error message.
b-kloss Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/abstractitensornetwork.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,11 @@ function hascommoninds(
::typeof(siteinds), A::AbstractITensorNetwork{V}, B::AbstractITensorNetwork{V}
) where {V}
Comment on lines 798 to 799
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
::typeof(siteinds), A::AbstractITensorNetwork{V}, B::AbstractITensorNetwork{V}
) where {V}
::typeof(siteinds), A::AbstractITensorNetwork, B::AbstractITensorNetwork
)

Copy link
Member

Choose a reason for hiding this comment

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

Same for check_hascommoninds.

for v in vertices(A)
!hascommoninds(siteinds(A, v), siteinds(B, v)) && return false
if isempty(siteinds(A, v))
!isempty(siteinds(B, v)) && return false
else
!hascommoninds(siteinds(A, v), siteinds(B, v)) && return false
end
Comment on lines +801 to +805
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by this change. It seems like this catches the case when A doesn't have site indices on vertex v, but B has site indices on vertex v. That seems like a case that hascommoninds should be working for. What goes wrong in hascommoninds in that case?

Copy link
Contributor Author

@b-kloss b-kloss Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When A doesn't have site indices on vertex v, but B has site indices on vertex v, it will return false, while it will continue the loop if both don't have site indices on vertex v. The false would indeed also be caught in the else clause, so it would indeed be more transparent to write

if isempty(siteinds(A,v)) && isempty(siteinds(B,v))
   continue
else
....

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, thanks. I see that is a strange corner case. See my comment #144 (comment), I think it would be simpler to just remove these functions entirely, since they are likely doing more harm than good, and we can reassess what kind of checks we really want in this functions, if any.

end
return true
end
Expand All @@ -815,9 +819,15 @@ function check_hascommoninds(
)
end
for v in vertices(A)
!hascommoninds(siteinds(A, v), siteinds(B, v)) && error(
"$(typeof(A)) A and $(typeof(B)) B must share site indices. On vertex $v, A has site indices $(siteinds(A, v)) while B has site indices $(siteinds(B, v)).",
)
if isempty(siteinds(A, v))
Copy link
Member

Choose a reason for hiding this comment

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

It seems like check_hascommoninds could just call hascommoninds(siteinds, A, B) and return nothing if it is true and error if it is false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

!(isempty(siteinds(B, v))) && error(
"$(typeof(A)) A and $(typeof(B)) B must share site indices. On vertex $v, A has no site indices while B has site indices $(siteinds(B, v)).",
)
else
!hascommoninds(siteinds(A, v), siteinds(B, v)) && error(
"$(typeof(A)) A and $(typeof(B)) B must share site indices. On vertex $v, A has site indices $(siteinds(A, v)) while B has site indices $(siteinds(B, v)).",
)
end
end
return nothing
end
Expand Down
5 changes: 4 additions & 1 deletion test/test_opsum_to_ttn.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@ end
Tttno = contract(Hsvd)
end
@test Tttno ≈ Tmpo rtol = 1e-6

Hsvd_lr = TTN(
Hlr, is_missing_site; root_vertex=root_vertex, algorithm="svd", cutoff=1e-10
)
Expand All @@ -257,6 +256,10 @@ end
Tmpo_lr = contract(Hsvd_lr)
end
@test Tttno_lr ≈ Tmpo_lr rtol = 1e-6
# ToDo: move to a more appropriate file, since this is not related to OpSum to TTN conversion
# test whether Hamiltonian contracts with itself (used to error due to missing siteindices)
@test isa(inner(Hsvd, Hsvd), Number)
Comment on lines +259 to +261
Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Also, it would be good to add tests for hascommoninds and check_hascommoninds with missing site indices.

end
end
end
return nothing
Loading