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

embed does not preserve the basis when a CompositeBasis with just a single site is used #27

Open
noh827 opened this issue Jul 30, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@noh827
Copy link

noh827 commented Jul 30, 2024

Suppose op is a LazyTensor whose basis is a CompositeBasis with just one site. Then the basis of dense(op) is not the same as the basis of op. Specifically the basis of dense(op) is a single-site non composite basis (e.g., NLevel(N=10) instead of [NLevel(N=10)]). This then causes an "incompatible basis" error in a more complex implementation of dense where it sums over many "densified" operators.

It looks like this is ultimately due to the implementation of the embed method here https://github.com/qojulia/QuantumInterface.jl/blob/main/src/embed_permute.jl#L55 and how the function tensor works for a single-site operator https://github.com/qojulia/QuantumInterface.jl/blob/main/src/tensor.jl#L7C1-L7C7.

Would it be possible to fix these methods so that the basis is preserved under operations like embed and dense?

@noh827 noh827 changed the title embed does not preserve the basis when a CompositeBasis with just a single site is used embed does not preserve the basis when a CompositeBasis with just a single site is used Jul 30, 2024
@noh827
Copy link
Author

noh827 commented Jul 30, 2024

@Krastanov for visibility

@amilsted amilsted added the bug Something isn't working label Jul 30, 2024
@amilsted
Copy link
Collaborator

amilsted commented Jul 30, 2024

I think this should be considered a bug: embed(my_basis, ...) should always return an operator with basis my_basis, or at least in a compatible basis.

A few ways we could go about this:

  1. have tensor(op) always return a new object in a (single-element) CompositeBasis
  2. introduce a coerce_basis(op, b) interface that returns op in the basis b and use it in embed
  3. make single-element CompositeBasis compatible with the non-wrapped basis (would require broader changes to basis handling)

PS: There's a related issue where it's possible to construct CompositeBasis backed by a Vector of bases, rather than a Tuple, but embed always gives us a tuple-backed CompositeBasis, which is not compatible with an equivalent vector-backed CompositeBasis. Another reason to maybe overhaul how we handle basis checks (they're not a good match for the type system!).

@amilsted
Copy link
Collaborator

amilsted commented Jul 31, 2024

I'm thinking adding a coerce_basis or cast_basis is probably the best path. This is an operation users will sometimes want anyway. We can still do 3 if we want, or more generally overhaul the basis system later, and 1 is a breaking change.

@Krastanov
Copy link
Collaborator

@akirakyle , pinging you here as this might also inform tangentially some of the design you are doing for #26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants