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

The BasisEmbedding template does not allow for all-ones or all-zero basis states #1112

Closed
josh146 opened this issue Feb 25, 2021 · 2 comments · Fixed by #1114
Closed

The BasisEmbedding template does not allow for all-ones or all-zero basis states #1112

josh146 opened this issue Feb 25, 2021 · 2 comments · Fixed by #1114
Assignees
Labels
bug 🐛 Something isn't working

Comments

@josh146
Copy link
Member

josh146 commented Feb 25, 2021

There is a bug in the input validation for the BasisEmbedding template. It should be checking that the basis state array only contains 0 and 1 integers, but the way it is currently coded, it also disallows all-ones and all-zero basis states:

https://github.com/PennyLaneAI/pennylane/blob/master/pennylane/templates/embeddings/basis.py#L54

@josh146 josh146 added the bug 🐛 Something isn't working label Feb 25, 2021
@antalszava
Copy link
Contributor

Indeed, the following raises an error when using BasisEmbedding:

import pennylane as qml
import numpy as np

dev = qml.device('default.qubit', wires=[0,1,2])

@qml.qnode(dev)
def circuit():
    qml.templates.embeddings.BasisEmbedding(np.array([1,1,1]), wires=[0,1,2])
    return qml.expval(qml.PauliZ(0))

circuit()
ValueError: Basis state must only consist of 0s and 1s; got [1, 1, 1]

However, this got me wondering: isn't BasisEmbedding identical to BasisStatePreparation? 🤔

For the latter, the check is correct and it seems as though it had the same functionality. Maybe @mariaschuld could also help here. If they are identical, we could maybe consider having BasisEmbedding depend on BasisStatePreparation and eventually deprecating it.

@mariaschuld
Copy link
Contributor

Yes, there is no real reason why we couldn't use BasisStatePreparation. I think I always thought it hacks the initial state, while I wanted a hardware-compatible solution for the embedding, but I guess this argument is not valid (any more). But this is a general decision, I propose to decouple it from this bug.

I made a fix in PR #1114...

@josh146 josh146 changed the title The BasisState template does not allow for all-ones or all-zero basis states The BasisEmbedding template does not allow for all-ones or all-zero basis states Feb 28, 2021
@josh146 josh146 linked a pull request Feb 28, 2021 that will close this issue
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

Successfully merging a pull request may close this issue.

3 participants