-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Utilities for approximate hardware noise #4671
Utilities for approximate hardware noise #4671
Conversation
|
||
|
||
@dataclass(frozen=True) | ||
class OpIdentifier: |
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.
Is this class really needed ? (having looked at other PRs as well).
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.
We could probably get away without it, but the result would be a lot messier. Here are the alternatives I considered:
- Just use
Operation
(it's hashable and contains the required info): this requires every currentOpIdentifier
reference to have some concept of "canonical" gates for each type supported in noise model generation (e.g. thePhasedXZGate
constructor requires(x|z|axis_phase)_exponent
args). This is considerably less convenient than just converting OpIDs to Operations during serialization. - Use a (Gate, qubits) tuple: this works about the same as
OpIdentifier
, but it's ultimately less readable to be sayingop_id[0]
andop_id[1:]
instead ofop_id.gate_type
andop_id.qubits
If you have any other recommendations, I'm happy to consider them, but my sense from working through this is that OpIdentifier
adds readability for relatively little cost. I opted not to surface OpIdentifier
at the cirq
package level to avoid any potential confusion.
eaa0597
to
53198f6
Compare
53198f6
to
55069a9
Compare
Needed some force-pushing to bring in the |
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.
Couple more comments
) | ||
|
||
def __contains__(self, item: Union[ops.Gate, ops.Operation]) -> bool: | ||
if isinstance(item, ops.Gate): |
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.
What would we do if we want an OpIdentifier for an Operation without a gate?
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.
None of the models from noise required that, so I opted not to support it. Is there a particular case you have in mind?
(CircuitOperations are a definite complication as far as matching is concerned, but I don't expect them to be used as the match target for an OpIdentifier.)
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.
In my use cases I had custom Operations that were not constructed using a Gate. I suppose the user could always generate a placeholder Gate object to identify any such Operation.
I can't give any native cirq Operation where this may be an issue since I am not so familiar with all the native cirq ops. One place you may have issues is with measurement operations. I.e. the output of cirq.measure has a gate output that is key dependent.
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.
That case should be OK - this only checks against qubits and gate types, both of which are defined explictly in the cirq.measure call. On a more general note, Cirq has been moving towards an "all operations have gates" structure (#4683), which should help with any remaining problem cases.
Pinging @MichaelBroughton for re-review. |
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.
LGTM after snake case fix in a few of the helper functions there.
Automerge cancelled: A required status check is not present. Missing statuses: ['Misc check', 'Pytest MacOS (3.7)', 'Pytest MacOS (3.8)', 'Pytest MacOS (3.9)', 'Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage'] |
* Remove files for subsequent PRs * Add coverage for OpId * Clarify if-case * snake_case_gamma Co-authored-by: Cirq Bot <craiggidney+github+cirqbot@google.com>
This PR is part of quantumlib#4640. It adds the `InsertionNoiseModel`, which injects noise based on a user-defined map. quantumlib#4671 is a prerequisite for this PR. The only files that need to be reviewed in this PR are: - `cirq-core/cirq/devices/...` - `__init__.py` - `insertion_noise_model[_test].py`
* Remove files for subsequent PRs * Add coverage for OpId * Clarify if-case * snake_case_gamma Co-authored-by: Cirq Bot <craiggidney+github+cirqbot@google.com>
This PR is part of quantumlib#4640. It adds the `InsertionNoiseModel`, which injects noise based on a user-defined map. quantumlib#4671 is a prerequisite for this PR. The only files that need to be reviewed in this PR are: - `cirq-core/cirq/devices/...` - `__init__.py` - `insertion_noise_model[_test].py`
* Remove files for subsequent PRs * Add coverage for OpId * Clarify if-case * snake_case_gamma Co-authored-by: Cirq Bot <craiggidney+github+cirqbot@google.com>
This PR is part of quantumlib#4640. It adds the `InsertionNoiseModel`, which injects noise based on a user-defined map. quantumlib#4671 is a prerequisite for this PR. The only files that need to be reviewed in this PR are: - `cirq-core/cirq/devices/...` - `__init__.py` - `insertion_noise_model[_test].py`
This PR is part of #4640. It adds utility functions and objects for use in noise model generation.
Key changes include:
decoherence_pauli_error
were copied directly from the existingNoiseProperties
class. These methods will be removed fromNoiseProperties
in an upcoming PR.decoherence_pauli_error
. This method is copied from internal noise-generation tools.OpIdentifier
class. This class is used as a key for noise-insertion maps.PHYSICAL_GATE_TAG
. This tag is applied to "real" gates during noise model generation to prevent sequential noise models from applying noise to noise generated by a previous noise model.