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.

Sign up for GitHub

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

change: Set default for shots argument in AwsDevice.run to None instead of -1. #129

Merged
merged 1 commit into from
Aug 11, 2020
Merged

change: Set default for shots argument in AwsDevice.run to None instead of -1. #129

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/braket/aws/aws_device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
# language governing permissions and limitations under the License.

from enum import Enum
from typing import Union
from typing import Optional, Union

import boto3
from networkx import Graph, complete_graph, from_edgelist
Expand Down Expand Up @@ -47,7 +47,6 @@ class AwsDevice(Device):
"d-wave": ["us-west-2"],
}

_DUMMY_SHOTS = -1
DEFAULT_SHOTS_QPU = 1000
DEFAULT_SHOTS_SIMULATOR = 0
DEFAULT_RESULTS_POLL_TIMEOUT = 432000
Expand Down Expand Up @@ -80,7 +79,7 @@ def run(
self,
task_specification: Union[Circuit, Problem],
s3_destination_folder: AwsSession.S3DestinationFolder,
shots: int = _DUMMY_SHOTS,
shots: Optional[int] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this make any difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-1 as a dummy value seems arbitrary, and has tripped up us once in a bug bash. That being said, it's not a big deal if you want to leave it as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, just wondering if this change is due to a customer-impacting bug, but it seems like the change is more for readability purposes. Please update the commit message to be prefixed with "change: ", as per https://github.com/aws/amazon-braket-sdk-python/blob/main/CONTRIBUTING.md#commit-your-change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks.

poll_timeout_seconds: int = DEFAULT_RESULTS_POLL_TIMEOUT,
poll_interval_seconds: int = DEFAULT_RESULTS_POLL_INTERVAL,
*aws_quantum_task_args,
Expand Down Expand Up @@ -132,7 +131,7 @@ def run(
See Also:
`braket.aws.aws_quantum_task.AwsQuantumTask.create()`
"""
if shots == AwsDevice._DUMMY_SHOTS:
if shots is None:
if "qpu" in self.arn:
shots = AwsDevice.DEFAULT_SHOTS_QPU
else:
Expand Down