-
Notifications
You must be signed in to change notification settings - Fork 118
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
Add results, state vector, amplitude, probability #60
Conversation
src/braket/circuits/results.py
Outdated
self._target = QubitSet(target) | ||
|
||
def to_ir(self) -> ir.Probability: | ||
return ir.Probability(targets=list(self.target)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.target
should be converted to a list of int
s; the local simulators will consume the output of to_ir
directly, and SDK classes should not be exposed there. Same applies to any other to_ir
definitions that contain an SDK class within the returned value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Qubit is a subclass of int. Even if the local simulators consume the output of to_ir directly, I don't think it would be an issue because it's still an int.
src/braket/circuits/__init__.py
Outdated
from braket.circuits.result_type import ResultType # noqa: F401 | ||
from braket.circuits.result_types import Amplitude, Probability, StateVector # noqa: F401 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not going to reopen the discussion about naming, but seems like having left Result (or ResultBase) and suffixing all the child classes with Result (such as AmplitudeResult) would be more accurate and prevent naming collisions should we need to add additional Amplitude, Probability, or StateVector classes that are something other than Results in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove this line and just import in result_types as result_types so that these can just be referred to as ResultType.Amplitude, etc. to avoid naming collisions in the circuits namespace. If users want they can also import in the classes directly and use them.
Issue #, if available:
*Description of changes:
build_files.tar.gz
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.