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

merge of AbstractQuantum should return a AbstractQuantum #277

Open
mofeing opened this issue Jan 5, 2025 · 2 comments
Open

merge of AbstractQuantum should return a AbstractQuantum #277

mofeing opened this issue Jan 5, 2025 · 2 comments
Labels
bug Something isn't working good first issue Good for newcomers triage Needs group consensus
Milestone

Comments

@mofeing
Copy link
Member

mofeing commented Jan 5, 2025

I just noticed that when doing the following:

ψ = rand(MPS; n=3) # hide
O = Product(fill(zeros(2,2),3)) # hide
exp_val = merge(ψ, O, ψ')

exp_val is a MPS and not a Quantum.

IMO it should return a Quantum in general.

@mofeing mofeing added bug Something isn't working good first issue Good for newcomers triage Needs group consensus labels Jan 5, 2025
@mofeing mofeing added this to the 0.8 milestone Jan 5, 2025
@mofeing mofeing modified the milestones: 0.8, 0.8.x Jan 15, 2025
@starsfordummies
Copy link
Contributor

is it as simple as replacing return a with return Quantum(a) in base.merge! ?

@mofeing
Copy link
Member Author

mofeing commented Jan 16, 2025

Yes, but something I spoke with @jofrevalles and @Todorbsc yesterday is that I'm thinking about making mutating methods only available for concrete types, not the abstract types. This way mutating methods like push!, pop!, replace! and merge! would not modify stuff implicitly which is the main source of bugs.

The reason is that is rare that you push! a Tensor to a MPS, or even that you replace a index label directly, but you require that functionality in other high-level functions like simple_update! or canonize!. Also, we already need to do some kind of interceptions because mutation in some level can make information in some other level invalid. For example, if you call replace!(::AbstractTensorNetwork, ::Pair{Symbol,Symbol}) to rename one index, you may have problems if you pass a Quantum because Quantum contains a mapping from indices to physical sites, so what we do is intercept it with another method replace!(::AbstractQuantum, ::Pair{Symbol,Symbol}) that updates this mapping and calls the more general method of AbstractTensorNetwork.

So for the fix you say, that will be true automatically because we won't use AbstractQuantum on merge! but just Quantum. The responsible of converting it to another type will be the method who called this; i.e. simple_update!, canonize!...

PS: Non-mutating methods like tensors, inds, sites, ... will still be inherited and work at all levels.

PS. 2: @starsfordummies sorry for the long reply, but this changes will allow us to have the new Circuit <: AbstractQuantum type required for the pTEBD and the new Stack type that can represent brakets, like for example <MPS|MPO|MPS>, all in 1 TN while tracking which tensors and indices belong to which TN (so you could easily do the transverse folding).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers triage Needs group consensus
Projects
None yet
Development

No branches or pull requests

2 participants