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

Add controlled unitary gate #1069

Merged
merged 23 commits into from
Feb 16, 2021
Merged

Add controlled unitary gate #1069

merged 23 commits into from
Feb 16, 2021

Conversation

trbromley
Copy link
Contributor

@trbromley trbromley commented Feb 5, 2021

Context:

Adds a new gate, ControlledQubitUnitary, which allows users to specify the matrix of a unitary, the control wire(s) and the target wire(s).

Description of the Change:

This gate has been added as an operation in pennylane/ops/qubit.py. It inherits from QubitUnitary and just performs some tweaks in the __init__.

Possible Drawbacks:

The underlying matrix could get quite large if the user specifies a lot of control wires. However this seems anyway expected if we are working in the matrix representation.

@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #1069 (1264ee2) into master (407517c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1069   +/-   ##
=======================================
  Coverage   97.72%   97.73%           
=======================================
  Files         154      154           
  Lines       11655    11679   +24     
=======================================
+ Hits        11390    11414   +24     
  Misses        265      265           
Impacted Files Coverage Δ
pennylane/devices/default_mixed.py 100.00% <ø> (ø)
pennylane/devices/default_qubit.py 100.00% <ø> (ø)
pennylane/ops/qubit.py 97.45% <100.00%> (+0.10%) ⬆️

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 407517c...1264ee2. Read the comment docs.

@trbromley trbromley self-assigned this Feb 9, 2021
@trbromley trbromley added the review-ready 👌 PRs which are ready for review by someone from the core team. label Feb 9, 2021
@josh146 josh146 requested review from thisac and removed request for josh146 February 9, 2021 17:40
Copy link
Contributor

@thisac thisac left a comment

Choose a reason for hiding this comment

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

Thanks @trbromley! I might be missing something about how this is supposed to work, but it seems like control/target wires chosen doesn't make a difference, causing the first wires to always be control with the remaining wires at the end targets. 🤔

pennylane/ops/qubit.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
tests/ops/test_qubit_ops.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Show resolved Hide resolved
@trbromley trbromley marked this pull request as ready for review February 10, 2021 12:32
trbromley and others added 2 commits February 10, 2021 07:33
@trbromley
Copy link
Contributor Author

Thanks @thisac for the review! Let me know if you're not persuaded regarding the wires question and we can discuss more.

Copy link
Contributor

@thisac thisac left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation @trbromley! It looks great. 💯

pennylane/ops/qubit.py Outdated Show resolved Hide resolved
Comment on lines 841 to 846
# the 4-qubit representation of the unitary if the control wires were [0, 1] and the target
# wires were [2, 3]
U_matrix = np.eye(16, dtype=np.complex128)
for i in range(4):
for j in range(4):
U_matrix[i + 12, j + 12] = U[i, j]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use block_diag(np.eye(12, dtype=np.complex128), U)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes good point, I wanted to do it in a way that's totally different from what's done in ControlledQubitUnitary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood! Not sure if that's necessary though, since we're not really testing block_diag here. 🤔

tests/ops/test_qubit_ops.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
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.

This is looking good @trbromley! 💯 Left some comments mostly related to testing, but nothing major

pennylane/ops/qubit.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
tests/ops/test_qubit_ops.py Outdated Show resolved Hide resolved
tests/ops/test_qubit_ops.py Outdated Show resolved Hide resolved
tests/test_operation.py Show resolved Hide resolved
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 to me! 💯

@trbromley trbromley merged commit a4060d6 into master Feb 16, 2021
@trbromley trbromley deleted the add_controlled_unitary branch February 16, 2021 13:33
trbromley added a commit that referenced this pull request Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-ready 👌 PRs which are ready for review by someone from the core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants