-
Notifications
You must be signed in to change notification settings - Fork 29
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 addControl
, clearControls
, removeControl
functions on Operation
#411
Conversation
This factors out the functionality previously implemented in #309 to 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.
Many thanks for the PR.
Can you maybe split this up even further?
I think both concepts are rather independent and still require some work on their own.
As for the control-part, I feel that you could take this a step further. Consider the following as a rough checklist
- I think the current concept of how this is implemented does not work in the general case. Imagine the following case
auto op = CompoundOperation{3};
op.emplace_back<StandardOperation>(3, 0, OpType::X);
op.setControls({1_pc});
op.setControls({2_pc});
The resulting gate would have two controls instead of just one. I believe it will become quite complicated to fix that general scenario. You would probably use the controls
member of the CompoundOperation to keep track of the controls and modify the methods that work on CompoundOperations to additionally factor those controls in. That's a rather big, non-local change though that I would like to avoid for now.
Depending on how you feel about it, I would maybe replace the current version of the new function with one that throws an error indicating that this is not handled currently and might lead to surprises.
- If not already, a similar warning should probably be raised in the
NonUnitaryOperatoin
class and you should carefully check whether this is properly handled in theClassicControlledOperation
class. - What you might want to do instead of the above is to add
addControl(const Control c)
andaddControls(const Controls& controls)
functions to theOperation
class and all derived classes. The second method should just reuse the first one to avoid code duplication. This more closely resembles the semantics of thecontrol
modifier from OQ3. TheaddControl
method should definitely contain some sanity checking, that none of the new controls is already a control or target of any existing gate. You can check that by running over the new controlsc
and callingop->actsOn(c)
. An error should be thrown in that case. Again, I would argue that these functions should return an error forNonUnitaryOperation
s as that concept does not really make sense there. - It would be great to test these various scenarios and make sure that everything works properly. This includes testing all different Operation types that should work and those that should not, including scenarios where the control to be added is already part of the gate.
As for the inverse
method, the current implementation is lacking quite some functionality. Again, a small checklist (you might want to use as starter for that second PR):
- The inverse of an operation is not simply derived by changing the size of the parameters. Instead, there are some operations that are self-inverse (e.g., I, X, Y, Z, H), some (non-parametrised) gates where the inverse is another gate we support (e.g., S, T, SX), and some where the inverse can be derived from some parameter changes (e.g., any rotation gate). This mapping needs to be provided somewhere and I believe the
Operation
class is the wrong place for that. I would argue that the base class method should be abstract (=0
) - In the
StandardOperation
class, there probably needs to be a rather long switch-case per operation (or type of operation) that handles inverting the individual operations. You should be able to group quite some of them together to reduce the number of switch cases that actually need an implementation. You should be able to derive these from https://github.com/cda-tum/mqt-core/blob/main/include/dd/Operations.hpp. In the best case, some of the potential code duplication between that file and the newly introducedinverse
method can be reduced. - In the
SymbolicOperation
class, there probably needs to be a little specialisation for parametrised gates that takes inspiration from theStandardOperation
class - I think you already nailed the
invert
operation for theCompoundOperation
. You might want to add a similar method to theQuantumComputation
class though. - In the
NonUnitaryOperation
class, an error should be thrown - In the
ClassicControlledOperation
class, you probably just want to defer to the underlying operation's inverse method - All of these scenarios should be tested to ensure that they work properly.
I hope this gives you some guidelines on how to proceed.
c524d9c
to
26848df
Compare
Please let me know if I should move the |
Hey 👋🏻 |
8774c7f
to
ff61243
Compare
This replaces the `setControls` function on `Operation` with an `addControls` function, which correctly handles adding controls on `CompoundOperations`. To replace controls, the two functions `clearControls` and `addControls` need to be called. Calling `addControls`/`clearControls` on `NonUnitaryOperation`s or adding controls that are already controls/targets throws an exception.
0c8be86
to
cc9a204
Compare
Operation
addControl
, clearControls
, removeControl
functions on Operation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #411 +/- ##
=======================================
+ Coverage 89.3% 89.5% +0.1%
=======================================
Files 98 102 +4
Lines 11624 11820 +196
Branches 2081 2095 +14
=======================================
+ Hits 10387 10580 +193
- Misses 1237 1240 +3
|
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.
Many thanks for this clean PR. I just went through everything and I only have minor feedback left that should be very easy to incorporate.
Small side note: You don't necessarily need to squash your commits and force push for your PRs. If PRs are small enough (such as this one), we simplify squash all commits upon merging.
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.
Thanks for the changes. One last round of feedback. Then, this should be good to merge.
Edit: It would be nice to get the patch coverage of the changes in this PR above the mark so that the CI check turns green ✅
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.
Many thanks! Looks great now ✅
Thank you for taking the time for the reviews! |
## Description This is a follow-up to #411 that fixes an oversight that was revealed by compiler warnings. The `Operation` class actually defines two `getControls` methods and #411 provided an override for a mixture of both, but not the actual methods. ## Checklist: <!--- This checklist serves as a reminder of a couple of things that ensure your pull request will be merged swiftly. --> - [x] The pull request only contains commits that are related to it. - [x] I have added appropriate tests and documentation. - [x] I have made sure that all CI jobs on GitHub pass. - [x] The pull request introduces no new warnings and follows the project's style guidelines. Signed-off-by: burgholzer <[email protected]>
Description
This PR replaces the
setControls
function onOperation
withaddControls
andclearControls
.Calling
clearControls
onCompoundOperation
s will throw an Exception, since doing so is not possible at the moment (we don't know if a control was pushed down from aCompoundOperation
or was there from the beginning.Checklist: