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

Support qml.sample() without specifying the observable #266

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

king-p3nguin
Copy link
Contributor

@king-p3nguin king-p3nguin commented Jun 7, 2024

Issue #, if available:
Related to #98

Description of changes:

  • qml.sample() can now be used without specifying the observable.

Example:

import pennylane as qml
from pennylane import numpy as np

dev_managed_sim = qml.device(
    "braket.aws.qubit",
    device_arn="arn:aws:braket:::device/quantum-simulator/amazon/sv1",
    wires=2,
    shots=10,
)

@qml.qnode(dev_managed_sim)
def circuit(a):
    qml.Hadamard(wires=0)
    qml.CNOT(wires=[0, 1])
    qml.RX(a, wires=1)
    # return qml.sample(qml.PauliZ(0))
    # return qml.sample()
    return qml.sample(wires=[0])


# x = np.array(0.543)
x = np.array([0.543, 0.3])
print(circuit(x))

output:

[[0 1 0 1 1 0 0 1 1 1]
 [1 0 1 0 1 0 0 1 1 0]]

Testing done:

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

Tests

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@king-p3nguin king-p3nguin marked this pull request as ready for review June 7, 2024 19:31
@king-p3nguin king-p3nguin requested a review from a team as a code owner June 7, 2024 19:31
@king-p3nguin king-p3nguin mentioned this pull request Jun 8, 2024
5 tasks
Copy link

codecov bot commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 10 lines in your changes missing coverage. Please review.

Project coverage is 99.18%. Comparing base (7b9f700) to head (f5a21fd).

Current head f5a21fd differs from pull request most recent head efcc1f7

Please upload reports for the commit efcc1f7 to get more accurate results.

Files Patch % Lines
src/braket/pennylane_plugin/translation.py 33.33% 8 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #266      +/-   ##
===========================================
- Coverage   100.00%   99.18%   -0.82%     
===========================================
  Files            7        7              
  Lines         1211     1223      +12     
  Branches       295      302       +7     
===========================================
+ Hits          1211     1213       +2     
- Misses           0        8       +8     
- Partials         0        2       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@rmshaffer rmshaffer left a comment

Choose a reason for hiding this comment

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

This change is an interesting improvement, but I don't think it addresses the issue #179.

The idea is to support a vector for the shots argument, like: https://docs.pennylane.ai/en/stable/code/api/pennylane.devices.default_qubit.DefaultQubit.html?highlight=shot_vector#pennylane.devices.default_qubit.DefaultQubit.shot_vector

The vector would be able to indicate a different number of shots to execute each circuit, rather than having every circuit executed the same number of shots.,

Comment on lines 570 to 575
if return_type is ObservableReturnTypes.Sample and observable is None:
if isinstance(measurement, qml.measurements.SampleMeasurement):
return tuple(
Sample(BraketObservable.Z(), target) for target in targets or measurement.wires
)
raise NotImplementedError(f"Unsupported measurement type: {type(measurement)}")
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of embedding this logic here, would it be possible to have _translate_observable return both an observable and targets, and have it fall back to these defaults if observable is none and/or targets is none? Might be slightly cleaner (and then maybe wouldn't require the C901 suppression above either, since the logic would be outside this function).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about that. The target cannot be specified if an observable is specified, so the branching will be based on whether the observable is None. Therefore, I believe that the overall logic will remain the same, and it will only be divided into more functions.

Comment on lines 711 to 716
if measurement.return_type is ObservableReturnTypes.Sample and observable is None:
if isinstance(measurement, qml.measurements.SampleMeasurement):
if targets:
return [m[targets] for m in braket_result.measurements]
return braket_result.measurements
raise NotImplementedError(f"Unsupported measurement type: {type(measurement)}")
Copy link
Contributor

Choose a reason for hiding this comment

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

same question here, would it be possible to embed this logic inside translate_result_type somehow?

@king-p3nguin king-p3nguin changed the title Support shots vectors in execute and batch_execute Support qml.sample() without specifying the observable Jun 15, 2024
@king-p3nguin
Copy link
Contributor Author

king-p3nguin commented Jun 15, 2024

Sorry for the misunderstanding. I do not think I will be able to provide a solution to issue #179 before the end of the unitary hack, so I will change the goal of the pull request to support qml.sample() without specifying the observable.

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