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

Skip over identity operations in "default.qubit" #2356

Merged
merged 4 commits into from
Mar 21, 2022

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented Mar 21, 2022

Currently, "default.qubit" handles identity operations by performing einsum with the identity.

This PR makes it so that identity operations are merely passed over.

@albi3ro albi3ro requested a review from antalszava March 21, 2022 19:47
@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #2356 (06dc6c1) into master (88e49ac) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2356   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files         242      242           
  Lines       18341    18343    +2     
=======================================
+ Hits        18235    18237    +2     
  Misses        106      106           
Impacted Files Coverage Δ
pennylane/devices/default_qubit.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88e49ac...06dc6c1. Read the comment docs.

Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Hi @albi3ro, great catch! It would be great to cover this functionality with tests too. For example, one could check that no internal methods (e.g., _apply_unitary_einsum) are being called for applying identities.

@@ -242,6 +242,8 @@ def _apply_operation(self, state, operation):
Returns:
array[complex]: output state
"""
if operation.base_name == "Identity":
return state
Copy link
Member

Choose a reason for hiding this comment

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

Are there any other "no-ops" to include here? e.g., things like a barrier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Barrier and WireCut don't define a matrix, so we don't have to worry about matrix operations with them. They get expanded away to nothing prior to execution. Identity, on the other hand, is an explicitly supported gate, so it won't get expanded away.

@albi3ro albi3ro requested a review from antalszava March 21, 2022 21:44
spy_unitary = mocker.spy(dev, "_apply_unitary")

res = dev._apply_operation(starting_state, op)
assert res is starting_state
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is makes sure that the state isn't just approximately the same values, but was literally unaffected by the method.

@albi3ro
Copy link
Contributor Author

albi3ro commented Mar 21, 2022

@antalszava test is added.

Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Looks good! 💯 Great catch. 🎉 Would this be applicable for default.mixed too?

Comment on lines +52 to +53
* `"default.qubit"` now skips over identity operators instead performing matrix multiplication
with the identity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `"default.qubit"` now skips over identity operators instead performing matrix multiplication
with the identity.
* `default.qubit` now skips over identity operators instead of performing matrix multiplication
with the identity.

@@ -2341,6 +2341,24 @@ def compute_matrix(*params, **hyperparams):
assert np.allclose(res_mat, op.get_matrix())
assert np.allclose(res_wires, wires)

def test_identity_skipped(self, mocker):
"""Test identity operation does not perform additional computations."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Test identity operation does not perform additional computations."""
"""Test that applying the identity operation does not perform any additional computations."""

@albi3ro albi3ro merged commit 88a4c21 into master Mar 21, 2022
@albi3ro albi3ro deleted the device-ignore-identity branch March 21, 2022 23:26
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.

None yet

3 participants