Fix replace! function when replacing a Tensor with itself in pair argument
#227
Conversation
src/TensorNetwork.jl
Outdated
| @@ -437,6 +437,9 @@ end | |||
| function Base.replace!(tn::AbstractTensorNetwork, pair::Pair{<:Tensor,<:Tensor}) | |||
| tn = TensorNetwork(tn) | |||
| old_tensor, new_tensor = pair | |||
|
|
|||
| old_tensor == new_tensor && return tn | |||
There was a problem hiding this comment.
mmm shouldn't this be === to check for egality, not equality?
| old_tensor == new_tensor && return tn | |
| old_tensor === new_tensor && return tn |
There was a problem hiding this comment.
No! Since for example, now it failed when we did replace!(tn, A => conj(A)), when A is a Tensor with a parent Array that are Real.
There was a problem hiding this comment.
Exactly! If eltype(A) <: Real, then A === conj(A)!
There was a problem hiding this comment.
conj(A) returns a new object with is equal to A. It would be like you say if it is conj! (not conj).
There was a problem hiding this comment.
You're right with this last point, but the bug in #225 is when it's replaced with the same tensor (i.e. old_tensor === new_tensor). new_tensor = copy(old_tensor) should work and there's nothing bad with it even if it really doesn't change the TN.
I try to avoid == if posible because == iterates over the whole arrays and checks for equality element-wise, while === just checks the objectids and you're done. Thus it avoids the overhead of iterating over everything, which can be costly on large arrays (specially when we start using distributed arrays).
There was a problem hiding this comment.
but the bug in #225 is when it's replaced with the same tensor (i.e.
old_tensor === new_tensor)
Yes, but I tried replace!(tn, A => conj(A)) and we got also the same error. I think this is because if you do conj(A) you still get the same objectid, which for me this is strange:
julia> objectid(A)
0xc7798e05eba779fe
julia> objectid(conj(A))
0xc7798e05eba779feBut whatever, okay! If that is the case I agree that we can have ===, I will push that commit now.
|
Awesome! I'm leaving one import thing (egality vs equality) and some grammatical fix requests. |
|
@mofeing I am merging this PR on monday. Let's enjoy the weekend :) |
Summary
This PR resolves #225 by fixing the problem with
replace!when the sameTensorappears in both positions of thepairargument. Previously, since we did not have any specific check for this case, thereplace!function had apush!that did not add the tensor since it was already present, but then, because we had adelete!after thepush!, theTensorgot deleted.We fixed this problem by adding a condition in the
replace!function that checks if the two tensors are equal, and if they are, the function returns theTensorNetworkwithout changes.Additionally (and importantly @bsc-quantic/software !), this PR also fixes the commented-out tests for the
replace!function, and we have extended these tests to cover the specific case that this PR addresses.