Refactor handling of gate matrices and inverses #752
Conversation
Joshy-R
force-pushed
the
refactor_handling_of_gate_matrices_and_inverses
branch
from
8c9a224
to
0c22ab7
Compare
Joshy-R
force-pushed
the
refactor_handling_of_gate_matrices_and_inverses
branch
from
0c22ab7
to
3d824d7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #752 +/- ##
=======================================
- Coverage 92.1% 92.0% -0.1%
=======================================
Files 125 126 +1
Lines 13770 13652 -118
Branches 2157 2164 +7
=======================================
- Hits 12684 12565 -119
- Misses 1086 1087 +1
*This pull request uses carry forward flags. Click here to find out more.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| oss << "DD for gate " << op->getName() << " not available!"; | ||
| throw qc::QFRException(oss.str()); | ||
| } | ||
| auto [type, theta, beta, _] = |
Check notice
Code scanning / CodeQL
Unused local variable Note
burgholzer
added
refactor
There was a problem hiding this comment.
Hey 👋
Many thanks for starting to work on this! Great to see someone taking this on!
I really like the solution with the flags for indicating the inverse type of a gate and indicating whether a gate is diagonal! Very elegant.
I added some inline comments that are mostlly minor except one bigger one that might trigger some further refactorings. However, I believe these might be worth it.
One thing on top of all of that: It would be great if you could address the clang-tidy complaints.
Typically these are posted as PR comments, but that feature does not work for PRs from forks. You can still see the warnings either in the check summary here: https://github.com/cda-tum/mqt-core/pull/752/checks or when looking through the "Files Changed" tab on GitHub.
Thanks again and let me know if anything is unclear!
| enum OpTypeFlags { | ||
| OP_TYPE_INV = 1, | ||
| OP_TYPE_DIAG = 2, | ||
| }; |
There was a problem hiding this comment.
enum class OpTypeFlag: uint8_t {
NONE = 0b00,
INV = 0b01,
DIAG = 0b10,
};How about the above suggestion? first of all creates a dedicated NONE entry that replaces the use of 0s throughout the code. Uses binary integers to more clearly illustrate the flag character. And makes this an enum class.
However, given that the arithmetic with different enums does seem to upset clang-tidy, it might be worth to simply define these as
constexpr unit8_t OP_TYPE_FLAG_NONE = 0b00;
constexpr unit8_t OP_TYPE_FLAG_INV = 0b01;
constexpr unit8_t OP_TYPE_FLAG_DIAG = 0b10;| OP_TYPE_DIAG = 2, | ||
| }; | ||
|
|
||
| constexpr static unsigned N_OP_TYPE_FLAGS_BITS = 2; |
There was a problem hiding this comment.
| constexpr static unsigned N_OP_TYPE_FLAGS_BITS = 2; | |
| constexpr static unsigned NUM_OP_TYPE_FLAG_BITS = 2; |
Slight naming preference for consistency.
| #define OP_TYPE_INV 0b1 | ||
| #define OP_TYPE_DIAG 0b10 |
There was a problem hiding this comment.
Do these really need to be defined here? Aren't these defined as enums anyway?
| case qc::T: case qc::Tdg: | ||
| case qc::V: case qc::Vdg: | ||
| case qc::SX: case qc::SXdg: | ||
| case qc::I: case qc::H: case qc::X: case qc::Y: case qc::Z: |
There was a problem hiding this comment.
This is not yet completely correct and clang-tidy rightfully complains here.
I, H, X, Y, Z are all self inverse. So these need not be XORed.
I am also not a big fan of fall-through switch statements, so I'd rather have an explicit return in each of the blocks.
|
|
||
| const auto type = op->getType(); | ||
| inline std::tuple<qc::OpType, fp, fp, fp> |
There was a problem hiding this comment.
I am not yet fully happy with this fixes kind of return type. In principle, gates could have an arbitrary number of parameters. The fact that currently supported gates onlly use up to three parameters is purely artificial and there are examples of gates with 4+ parameters that we might want to support in the future. Do you see any kind of way of supporting that use case here? Either via variadic lists or vectors of parameters or something completely different..
Maybe it would even be better to take this one step further and represent an operation by its number of qubits, its type, and its number of parameters. Then one could probably transform the code here to a very uniform style and consolidate it quite a bit.
Adding even one step further, this might be worth pulling from the DD submodule to the ir/operations module as a whole given the general usefulness of getting the (inverse) matrix for a given StandardOperation.
Description
These commits change how matrices are created from an OpType that reduces the number of switch statements necessary.
The downside of this approach is that some functions can be called with arguments that do not affect the returned value. As the user never calls these functions, I see this as acceptable.
I also removed the CX_MAT and CZ_MAT, as they are only used in tests and do not occur in any other repository.
Fixes #484
Checklist:
I have not squashed the commits to make them easier to review, but I can squash them if desired.