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

Error when creating qnode with backprop and finite-shots #1588

Merged
merged 22 commits into from
Sep 10, 2021

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented Aug 24, 2021

Fixes Issue #1481

Right now, you can create a qnode that uses finite shots and backpropagation. This is fine on qnode execution on the forward pass, but will always fail and raise some sort of unhelpful error message of the backward pass/ gradient computation.

For example:

dev = qml.device('default.qubit.autograd', shots=10, wires=1)

@qml.qnode(dev, interface='autograd', diff_method='backprop')
def circ(x):
    qml.RX(x, wires=0)
    return qml.expval(qml.PauliZ(0))

## works fine
circ(0.1)

# incomprehensible error
qml.grad(circ)(0.1)

For context, that error is:

ValueError: setting an array element with a sequence.

With this change, during QNode creation, a UserWarning is raise. I decided to use a warning instead of a Error, as users may still want to calculate finite shot forward passes on an interface device. This way, they at least have an explanation for the strange errors they will end up getting.

@albi3ro albi3ro added the bug 🐛 Something isn't working label Aug 24, 2021
@albi3ro albi3ro requested a review from trbromley August 24, 2021 15:31
@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@antalszava
Copy link
Contributor

[ch8003]

@codecov
Copy link

codecov bot commented Aug 26, 2021

Codecov Report

Merging #1588 (5f3eee2) into master (67a5daf) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1588   +/-   ##
=======================================
  Coverage   99.14%   99.14%           
=======================================
  Files         196      196           
  Lines       14245    14247    +2     
=======================================
+ Hits        14123    14125    +2     
  Misses        122      122           
Impacted Files Coverage Δ
pennylane/qnode.py 98.69% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67a5daf...5f3eee2. Read the comment docs.

Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Hi @albi3ro, thanks! 💯 Had one major comment for a test case, the rest is looking good (minor suggestions).

tests/tape/test_qnode.py Outdated Show resolved Hide resolved
dev = qml.device("default.qubit", wires=1, shots=3)

with pytest.raises(qml.QuantumFunctionError, match="Devices with finite shots"):
QNode._validate_backprop_method(dev, "doesnt matter")
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if it's worth inputting valid interface names here. Just by looking at the test case, the error might arise due to the invalid interface. Contrary to that, we'd like to make sure that we get an error with every valid interface. So maybe good to parameterize the test.

pennylane/qnode.py Outdated Show resolved Hide resolved
.github/CHANGELOG.md Outdated Show resolved Hide resolved
@albi3ro albi3ro requested review from thisac and removed request for trbromley August 31, 2021 18:06
@albi3ro albi3ro added the WIP 🚧 Work-in-progress label Aug 31, 2021
@albi3ro albi3ro removed the request for review from thisac August 31, 2021 18:11
@antalszava
Copy link
Contributor

Hi @albi3ro what's the status on this?

I decided to use a warning instead of a Error, as users may still want to calculate finite shot forward passes on an interface device.

Could they simply just evaluate the QNode without trying to differentiate it? If so, maybe we could refer to that in the error message.

@albi3ro albi3ro requested a review from rmoyard September 9, 2021 18:00
Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Great, I've left some comments. 💯 I think we should add a new test, because a deleted test that was previously covering some jax functionalities was deleted.

@@ -342,6 +342,12 @@ def _validate_backprop_method(device, interface):
qml.QuantumFunctionError: if the device does not support backpropagation, or the
interface provided is not compatible with the device
"""
if device.shots is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Great 👍


dev = qml.device("default.qubit", wires=1, shots=3)

with pytest.raises(qml.QuantumFunctionError, match="Devices with finite shots"):
Copy link
Contributor

@rmoyard rmoyard Sep 9, 2021

Choose a reason for hiding this comment

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

Nice test 💯

return qml.sample(qml.PauliZ(0))

a = tf.Variable(0.54)
with pytest.raises(qml.QuantumFunctionError, match="Devices with finite shots"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep this test? Maybe changing the description to better fit the error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we test elsewhere that this error is raised, I'm deleting this test.

@albi3ro
Copy link
Contributor Author

albi3ro commented Sep 10, 2021

@rmoyard what do we need to add?

@albi3ro albi3ro added review-ready 👌 PRs which are ready for review by someone from the core team. and removed WIP 🚧 Work-in-progress labels Sep 10, 2021
@rmoyard
Copy link
Contributor

rmoyard commented Sep 10, 2021

@albi3ro I think one the test using jax was deleted. default_qubit_jax.py line 269 more precisely we should add something covering this line.

Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Great, it seems good to me 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working review-ready 👌 PRs which are ready for review by someone from the core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants