[ITensor] [Bug] Fix replaceinds for long inputs
#1098
Conversation
This reverts commit 030862a.
mtfishman
changed the title
replaceinds for long inputs
There was a problem hiding this comment.
The method:
function replaceinds(is::Indices, rep_inds::Vector{<:Pair{<:Index,<:Index}})
return replaceinds(is, rep_inds...)
endis also potentially problematic since splatting with ... has similar to using Tuple when it comes to performance for long lists (i.e. we should avoid splatting long lists).
I think we could just use this instead:
function replaceinds(is::Indices, rep_inds::Vector{<:Pair{<:Index,<:Index}})
return replaceinds(is, first.(rep_inds), last.(rep_inds))
endThere was a problem hiding this comment.
Also, I would rewrite:
function replaceinds(is::Indices, rep_inds::Tuple{Vararg{Pair{<:Index,<:Index}}})
return replaceinds(is, rep_inds...)
endas:
function replaceinds(is::Indices, rep_inds::Tuple{Vararg{Pair{<:Index,<:Index}}})
return replaceinds(is, first.(rep_inds), last.(rep_inds))
endThere was a problem hiding this comment.
Another simplification I would suggest is changing:
function replaceinds(is::Indices, rep_inds::Pair{<:Index,<:Index}...)
return replaceinds(is, zip(rep_inds...)...)
endto:
function replaceinds(is::Indices, rep_inds::Pair{<:Index,<:Index}...)
return replaceinds(is, first.(rep_inds), last.(rep_inds))
end(The zip version is too clever by half.)
Another thing I noticed looking back through this code is that we might have to add a missing method:
replaceinds(is::Indices, i1::Index, i2::Index) = replaceinds(is, (i1,), (i2,))for trying to replace a single Index.
| @@ -557,7 +557,7 @@ function replaceinds(is::Indices, rep_inds::Tuple{Vararg{Pair{<:Index,<:Index}}} | |||
| end | |||
|
|
|||
| function replaceinds(is::Indices, rep_inds::Pair) | |||
| return replaceinds(is, Tuple(first(rep_inds)) .=> Tuple(last(rep_inds))) | |||
| return replaceinds(is, first(rep_inds) .=> last(rep_inds)) | |||
There was a problem hiding this comment.
I wonder if we can just use:
return replaceinds(is, first(rep_inds), last(rep_inds))instead, which would then directly call the core version replaceinds(is::Indices, inds1, inds2). The logic of this code is a bit spaghetti-like, it's hard to track what is calling what and how it finally gets to the core implementation (which is my fault, this is some very early-days ITensors.jl code which has not been revisited in a while).
Description
There seems to be something peculiar with the Tuple structure. It would appear that Tuples store 100 values at a time in a structure so making a tuple of 101 elements will make 2 data structures
(1, 2, ..., 100)and(101, ). Because a Tuple of a single element has a dangling comma, operations like.=>will try to operate on the 102 element which doesn't exist.Fixes #1078
We can fix this easily be doing the element-wise operations in a vector