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.

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

Fix conj to use deepcopy instead of copy #229

Closed
wants to merge 1 commit into from
Closed

Conversation

jofrevalles
Copy link
Member

Summary

In this PR, we change Base.conj function to use deepcopy instead of copy to prevent errors when manipulating tensor networks within an @unsafe_region macro block.

The regular copy leads to problem since it calls TensorNetworks(tensors(tn)), which throws an error since there are inconsistent indices. Before the recent commit a241967 that refactored the conj and adjoint functions, we also had a deepcopy inside conj which prevent this problem.

Example

With this PR, we can do something like the following code without any problem:

julia> using Tenet

julia> tn = TensorNetwork([
           Tensor(ones(2, 2), [:a, :b]),
           Tensor(ones(2, 2), [:b, :c])
           ])
TensorNetwork (#tensors=2, #inds=3)

julia> Tenet.@unsafe_region tn begin
           c = Tensor(ones(3, 2), [:c, :d])

           push!(tn, c)
           conj(tn)
           pop!(tn, c)
       end
3×2 Tensor{Float64, 2, Matrix{Float64}}:
 1.0  1.0
 1.0  1.0
 1.0  1.0

@jofrevalles
Copy link
Member Author

jofrevalles commented Nov 5, 2024

@mofeing Okay I got another idea, we can keep the copy but add modify it like this:

function Base.copy(tn::TensorNetwork)
    if is_unsafe_region[]
        new_tn = TensorNetwork([])
        for tensor in tensors(tn)
            push!(new_tn, copy(tensor))
        end

        return new_tn
    else
        return TensorNetwork(tensors(tn))
    end
end

Since push! accounts for the unsafe_region. Or we can directly add that conditional inside the TensorNetwork constructor.

@mofeing
Copy link
Member

mofeing commented Nov 5, 2024

Okay, I like this idea more. The one about adding the check inside TensorNetwork constructor.

@jofrevalles
Copy link
Member Author

Okay, I like this idea more. The one about adding the check inside TensorNetwork constructor.

@mofeing If we do this, then we should agree to change the macro to my second option described in issue #230. For me, it makes sense.

@mofeing
Copy link
Member

mofeing commented Nov 6, 2024

Okay, I like the idea but we have to discuss consequences (e.g. we might have problems when multithreading). Add it to the agenda for today's meeting please.

@jofrevalles
Copy link
Member Author

@mofeing Since now we fixed the problems with copy inside some @unsafe_region in PR #233, now we don't need this PR.

@mofeing
Copy link
Member

mofeing commented Nov 13, 2024

Ah really? Nice!

@mofeing mofeing deleted the fix/conj branch November 13, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants