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

Make parametric controlled operators inherit from ControlledOp #5069

Merged
merged 42 commits into from
Jan 23, 2024

Conversation

astralcai
Copy link
Contributor

@astralcai astralcai commented Jan 15, 2024

Context:

Many controlled operations have been added to PennyLane over the years, such as CNOT, CRY, CZ, etc. For code tidiness, we would like to make them all inherit from the ControlledOp base.

Description of the Change:

CRX, CRY, CRZ, CRot, ControlledPhaseShift are now:

  1. Moved from pennylane.ops.qubit to pennylane.ops.op_math
  2. Inherit from ControlledOp

Calling qml.ctrl on RX, RY, RZ, Rot, and PhaseShift with a single control wire will return gates of types CRX, CRY, etc. as opposed to a general Controlled operator.

The decomposition of an operator created with calling qml.ctrl on RX, RY, RZ, Rot, PhaseShift with a single control wire will now be the full decomposition instead of a single controlled gate. For example:

>>> qml.ctrl(qml.RX(0.123, wires=1), control=0).decomposition()
[CRX(0.123, wires=[0, 1])]

Will become:

>>> qml.ctrl(qml.RX(0.123, wires=1), control=0).decomposition()
[
  RZ(1.5707963267948966, wires=[1]), 
  RY(0.0615, wires=[1]), 
  CNOT(wires=[0, 1]), 
  RY(-0.0615, wires=[1]), 
  CNOT(wires=[0, 1]), 
  RZ(-1.5707963267948966, wires=[1])
]

Benefits:

Code cleanliness.

Related GitHub Issues:

#1447

@astralcai astralcai changed the title Make parametric controlled operators inherit from ControlledOp [WIP] Make parametric controlled operators inherit from ControlledOp Jan 15, 2024
@trbromley
Copy link
Contributor

Given the size of the diff, could it make sense to split this up into smaller PRs?

@astralcai
Copy link
Contributor Author

Given the size of the diff, could it make sense to split this up into smaller PRs?

The size of the diff is actually smaller than it looks. We're moving these controlled operators from ops.qubit to ops.op_math, which is probably why it looks like a lot.

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c0ce3ce) 99.68% compared to head (c6694a8) 99.67%.
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5069      +/-   ##
==========================================
- Coverage   99.68%   99.67%   -0.02%     
==========================================
  Files         394      392       -2     
  Lines       35720    35575     -145     
==========================================
- Hits        35609    35460     -149     
- Misses        111      115       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@astralcai astralcai changed the title [WIP] Make parametric controlled operators inherit from ControlledOp Make parametric controlled operators inherit from ControlledOp Jan 17, 2024
@astralcai astralcai marked this pull request as ready for review January 17, 2024 18:19
@astralcai astralcai requested a review from a team January 17, 2024 18:19
Copy link
Contributor

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

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

a few little comments, but otherwise this is looking good!

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/ops/op_math/__init__.py Show resolved Hide resolved
pennylane/ops/op_math/controlled.py Show resolved Hide resolved
astralcai and others added 2 commits January 19, 2024 10:59
Co-authored-by: Matthew Silverman <matthews@xanadu.ai>
@Alex-Preciado
Copy link
Contributor

Linking to corresponding Shortcut story: [sc-53529]

Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

Looks great! 🎉 Thanks for handling all of this :)

@astralcai astralcai requested a review from a team January 22, 2024 21:47
Copy link
Contributor

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

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

🕺 🕺

@astralcai astralcai enabled auto-merge (squash) January 22, 2024 22:14
@astralcai astralcai merged commit d94c40c into master Jan 23, 2024
35 checks passed
@astralcai astralcai deleted the ctrl-par-ops branch January 23, 2024 14:44
astralcai added a commit that referenced this pull request Feb 1, 2024
**Context:**
All controlled operations should inherit from the general Controlled
class, and the decomposition of controlled operations is not consistent
for custom and non-custom controlled operations. This is a continuation
of #5069

This is the first PR out of two for this rework. The second PR will
focus on making sure that all custom controlled operations inherit from
Controlled for more consistent inheritance structure.

**Description of the Change:**
- Make `MultiControlledX` inherit from ControlledOp.
- `qml.ctrl` called on operators with custom controlled versions will
return instances of the custom class.
- Special handling of `PauliX` based controlled operations (`PauliX`,
`CNOT`, `Toffoli`, `MultiControlledX`)
- Calling `qml.ctrl` on one of these operators will always resolve to
the best option in `CNOT`, `Toffoli`, or `MultiControlledX` depending on
the number of control wires and control values.
- `qml.ctrl` will flatten nested controlled operators to a single
multi-controlled operation.
- Controlled operators with a custom controlled version decomposes like
how their controlled counterpart decomposes, as opposed to decomposing
into their controlled version.
- Special handling of `PauliX` based controlled operations: e.g.,
`Controlled(CNOT([0, 1]), [2, 3])` will have the same decomposition
behaviour as a `MultiControlledX([2, 3, 0, 1])`

**Benefits:**
Cleaner code and more consistent behaviour

**Possible Drawbacks:**
Change of decomposition behaviour may cause issues.
~For `MultiControlledX`, the `wires` attribute now refers to all wires,
as in `control_wires + target_wire + work_wires`, to access only the
`control_wires + target_wires`, use the `active_wires` attribute.~

**Related GitHub Issues:**
#5069
#1447

**Related Shortcut Stories**
[sc-55949]
[sc-55131]
[sc-55358]

---------

Co-authored-by: Christina Lee <christina@xanadu.ai>
Co-authored-by: Matthew Silverman <matthews@xanadu.ai>
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.

5 participants