-
Notifications
You must be signed in to change notification settings - Fork 612
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
Resource methods for adjoints, controlls, and pows. #6648
base: master
Are you sure you want to change the base?
Conversation
…nnylane into resource_multi_qubit
…ane into resource_symbolic_ops
…nnylane into resource_multi_qubit
…ane into resource_controlled_gates
…ane into resource_symbolic_decomps
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.
Looks mostly good. I left a few comments. I'll give this another look once you are finished adding tests.
@staticmethod | ||
def adjoint_resource_decomp() -> Dict[re.CompressedResourceOp, int]: | ||
return {} | ||
|
||
@staticmethod | ||
def controlled_resource_decomp( | ||
num_ctrl_wires, num_ctrl_values, num_work_wires | ||
) -> Dict[re.CompressedResourceOp, int]: | ||
return {} | ||
|
||
@staticmethod | ||
def pow_resource_decomp(z) -> Dict[re.CompressedResourceOp, int]: | ||
return {} | ||
|
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 think these should return {cls.resource_rep(): 1}
. This comment applies to the rest of the codebase as well. I think only the identity's resource decomposition should return {}
and anything else currently returning {}
should be changed to return {re.ResourceIdentity.resource_rep(): 1}
. Users might want to track how many times the identity operator shows up in their circuit, but this can't happen with return {}
. The performance gain in returning {}
is probably negligible.
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.
Changed 👍🏼
def controlled_resource_decomp( | ||
num_ctrl_wires, num_ctrl_values, num_work_wires | ||
) -> Dict[re.CompressedResourceOp, int]: | ||
if num_ctrl_values == 0: | ||
return {re.ResourcePhaseShift.resource_rep(): 1} | ||
raise re.ResourcesNotDefined |
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.
Do we not need to check if num_ctrl_wires == 1
? or do all multi controlled global phases decompose to the same phase shift operator?
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 modified the decomposition to be general now
gate_types = {rx: 1, ry: 1, rz: 1} | ||
gate_types = {ry: 1, rz: 2} |
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.
Why is this change needed?
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 think it was just implemented incorrectly?
if num_ctrl_values == 0: | ||
cnot = re.ResourceCNOT.resource_rep() | ||
ctrl_rz = re.ResourceControlled.resource_rep( | ||
base_class=re.ResourceRZ, | ||
base_params={}, | ||
num_ctrl_wires=num_ctrl_wires, | ||
num_ctrl_values=num_ctrl_values, | ||
num_work_wires=num_work_wires, | ||
) | ||
|
||
gate_types = {} | ||
gate_types[cnot] = 2 * (num_wires - 1) | ||
gate_types[ctrl_rz] = 1 | ||
|
||
return gate_types |
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.
Can we not handle num_ctrl_values > 0
by adding X gates?
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 can! However if you look at the logic in re.ResourceControlled
it first tries to expand the op, if it encounters an error, it adds the Xs and then tries the controlled method again. So here I am relying on that logic to add the Xs
num_work_wires, | ||
pauli_word, | ||
) -> Dict[re.CompressedResourceOp, int]: | ||
if num_ctrl_values == 0: |
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.
Same comment as above
num_ctrl_values, | ||
num_work_wires, | ||
) -> Dict[re.CompressedResourceOp, int]: | ||
if num_ctrl_values == 0: |
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.
Same
re.ResourceS.resource_rep(): 4, | ||
re.ResourceS.resource_rep(): 1, |
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.
This is because previously we were using S to count both S and Adjoint(S)?
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.
Yes
"""Tests for ResourceX""" | ||
|
||
def test_resources(self): | ||
"""Test that ResourceT does not implement a decomposition""" |
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.
Remove this comment
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.
Removed 👍🏼
No description provided.