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

Avoid in-place operations in adjoint and conj functions #226

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jofrevalles
Copy link
Member

Summary

This PR modifies the adjoint and conj functions for AbstractQuantum and AbstractTensorNetwork types to prevent errors when tensors contain immutable arrays. Instead of performing in-place operations, the functions now return a modified copy of the input.

Some tensors within a TensorNetwork may have immutable arrays in their parent structures. Performing in-place operations like conj! or adjoint! on these tensors can lead to errors because immutable arrays do not support the setindex! operation required for in-place modification.

Example

julia> using Tenet

julia> using YaoBlocks

julia> c = YaoBlocks.chain(2)

julia> c = YaoBlocks.chain(2, c, put(1 => X))

julia> c = YaoBlocks.chain(2, c, put(2 => X))

julia> c = YaoBlocks.chain(2, c, put(1 => X))

julia> allzeros = Quantum(Product(fill([1, 0], 2)))
Quantum (inputs=0, outputs=2)

julia> state_qtn = merge(allzeros, Quantum(c))
Quantum (inputs=0, outputs=2)
Before this PR:
julia> state_qtn'
ERROR: BoundsError: attempt to access 2×2 LuxurySparse.SDPermMatrix{ComplexF64, Int64, Vector{ComplexF64}, Vector{Int64}} at index [1, 1]
Stacktrace: ...
Now:
julia> state_qtn'
Quantum (inputs=2, outputs=0)

src/Quantum.jl Outdated Show resolved Hide resolved
src/TensorNetwork.jl Outdated Show resolved Hide resolved
Copy link
Member

@mofeing mofeing left a comment

Choose a reason for hiding this comment

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

Are there already tests for adjoint and conj? I guess that yes but not sure right now.

@jofrevalles
Copy link
Member Author

Are there already tests for adjoint and conj? I guess that yes but not sure right now.

I am writting these right now.

@jofrevalles
Copy link
Member Author

By the way, these errors appear now due to the issue reported in #225, since when we do conj now we call replace! inside, and that errors when the arrays of the tensors are Real, since then conj(tensor)=tensor.

See:

julia> mps = rand(Chain, Open, State, n=6, χ=10)
MPS (inputs=0, outputs=6)

julia> mps'
ERROR: Index A must be open
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] addsite!(tn::Quantum, site::Site{1}, index::Symbol)
   @ Tenet ~/git/Tenet.jl/src/Quantum.jl:279
 [3] adjoint(tn::Quantum)
   @ Main ./REPL[57]:8
 [4] adjoint(chain::Chain)
   @ Tenet ~/git/Tenet.jl/src/Ansatz/Chain.jl:209
 [5] top-level scope
   @ REPL[74]:1

julia> mps = rand(Chain, Open, State, n=6, χ=10, eltype=ComplexF64)
MPS (inputs=0, outputs=6)

julia> mps'
MPS (inputs=6, outputs=0)

@jofrevalles
Copy link
Member Author

When the PR #227 is merged, I will cherry-pick the fixing commits to ensure everything works well.

@mofeing
Copy link
Member

mofeing commented Oct 31, 2024

When the PR #227 is merged, I will cherry-pick the fixing commits to ensure everything works well.

You don't need to cherry-pick if the PR is merged??

@jofrevalles
Copy link
Member Author

When the PR #227 is merged, I will cherry-pick the fixing commits to ensure everything works well.

You don't need to cherry-pick if the PR is merged??

Just to run the tests before merge.

@mofeing
Copy link
Member

mofeing commented Nov 1, 2024

Ah, you don't need to cherry-pick but to rebase on top of master.

Actually, cherry-picking in that case can lead to merge conflicts.

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