Improve compatibility with tensor networks without internal vertices #144
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
==========================================
- Coverage 73.62% 73.55% -0.08%
==========================================
Files 70 70
Lines 4118 4122 +4
==========================================
Hits 3032 3032
- Misses 1086 1090 +4 ☔ View full report in Codecov by Sentry. |
| if isempty(siteinds(A, v)) | ||
| !isempty(siteinds(B, v)) && return false | ||
| else | ||
| !hascommoninds(siteinds(A, v), siteinds(B, v)) && return false | ||
| end |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
....
There was a problem hiding this comment.
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.
| !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)) |
There was a problem hiding this comment.
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.
| ::typeof(siteinds), A::AbstractITensorNetwork{V}, B::AbstractITensorNetwork{V} | ||
| ) where {V} |
There was a problem hiding this comment.
| ::typeof(siteinds), A::AbstractITensorNetwork{V}, B::AbstractITensorNetwork{V} | |
| ) where {V} | |
| ::typeof(siteinds), A::AbstractITensorNetwork, B::AbstractITensorNetwork | |
| ) |
| # 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) |
There was a problem hiding this comment.
Agreed. Also, it would be good to add tests for hascommoninds and check_hascommoninds with missing site indices.
|
@b-kloss another approach here would be to just remove |
|
This was addressed in #148. |
This PR addresses some issues related to using Tensor Networks where the
vertex_dataat some vertices is empty, e.g.Index[]. In particular the behaviour ofcheck_hascommonindswhich is for example called when computing an inner product between two tensor networks is now comparing emptyvertex_dataas equal. A test has been added that verifies that an inner product can be taken.ToDo:
TTN(s,"Up")is currently not functional ifsis anIndsNetworkwhere at least one vertex has emptyvertex_data.