Refactor handling of gate matrices and inverses #752

Joshy-R · 2024-11-14T10:21:07Z

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:

The pull request only contains commits that are related to it.
I have added appropriate tests and documentation.
I have made sure that all CI jobs on GitHub pass.
The pull request introduces no new warnings and follows the project's style guidelines.

I have not squashed the commits to make them easier to review, but I can squash them if desired.

codecov · 2024-11-14T11:32:22Z

Codecov Report

Attention: Patch coverage is 92.10526% with 18 lines in your changes missing coverage. Please review.

Project coverage is 92.1%. Comparing base (7f8c578) to head (337ba29).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
include/mqt-core/dd/Operations.hpp	82.6%	12 Missing ⚠️
src/dd/GateMatrixDefinitions.cpp	96.8%	4 Missing ⚠️
include/mqt-core/ir/operations/OpType.hpp	83.3%	1 Missing ⚠️
src/dd/FunctionalityConstruction.cpp	50.0%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main    #752    +/-   ##
======================================
  Coverage   92.1%   92.1%            
======================================
  Files        125     125            
  Lines      13790   13665   -125     
  Branches    2164    2172     +8     
======================================
- Hits       12702   12596   -106     
+ Misses      1088    1069    -19

Flag	Coverage Δ		*Carryforward flag
cpp	`91.9% <92.1%> (+<0.1%)`	⬆️
python	`99.7% <ø> (ø)`		Carriedforward from 7f8c578

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines	Coverage Δ
include/mqt-core/dd/FunctionalityConstruction.hpp	`100.0% <100.0%> (ø)`
include/mqt-core/dd/Simulation.hpp	`100.0% <100.0%> (ø)`
include/mqt-core/ir/operations/Operation.hpp	`79.7% <100.0%> (-0.3%)`	⬇️
src/circuit_optimizer/CircuitOptimizer.cpp	`90.5% <100.0%> (+0.2%)`	⬆️
src/dd/NoiseFunctionality.cpp	`97.0% <100.0%> (-0.1%)`	⬇️
src/dd/Operations.cpp	`98.6% <100.0%> (ø)`
src/dd/Simulation.cpp	`95.8% <100.0%> (+<0.1%)`	⬆️
include/mqt-core/ir/operations/OpType.hpp	`87.0% <83.3%> (-4.5%)`	⬇️
src/dd/FunctionalityConstruction.cpp	`90.6% <50.0%> (ø)`
src/dd/GateMatrixDefinitions.cpp	`96.8% <96.8%> (ø)`
... and 1 more

... and 15 files with indirect coverage changes

include/mqt-core/dd/Operations.hpp

burgholzer

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!

include/mqt-core/ir/operations/OpType.hpp

include/mqt-core/ir/operations/OpType.inc

include/mqt-core/dd/Operations.hpp

burgholzer

Oops, clicked the wrong button 🙃

burgholzer

Many thanks for the iteration here. This is looking great already and I like where this is heading.
I went through everything in detail again and added my feedback inline. Hope that makes sense. Please let me know if it doesn't!

include/mqt-core/ir/operations/OpType.inc

include/mqt-core/ir/operations/OpType.hpp

include/mqt-core/dd/GateMatrixDefinitions.hpp

include/mqt-core/dd/Operations.hpp

src/dd/GateMatrixDefinitions.cpp

test/dd/test_package.cpp

…_MAT and CZ_MAT

refractor functions to receive Operation as const referece instead of pointer refractor getStandardOperationDD to receive targets as vector, remove overload that takes two targets and inline helper function as it now only has a single use

Joshy-R · 2024-12-15T23:01:29Z

I tried to implement your thoughts as good as possible. One change necessary to transition the test to retrieve the DD via the getDD and the StandardOperation class was to pass the standard operations as a const reference instead of a pointer. Also, this is closer to the semantics, as the code assumes the pointer is not null, and the code does not use the object's location information. This way, instead of writing dd::getDD(&(qc::StandardOperation){ 0, qc::RZ, {dd::PI_4}}, dd), we can write dd::getDD(qc::StandardOperation(0, qc::RZ, {dd::PI_4}), dd), which is a bit easier to read.

I hope this is a further improvement to the previous draft.

Joshy-R · 2024-12-16T12:08:02Z

I am also trying to understand why one CI build fails, but I don't see how my changes produced this error. Would you happen to have an idea, or would you be able to help me with that?

burgholzer · 2024-12-16T13:06:02Z

I am also trying to understand why one CI build fails, but I don't see how my changes produced this error. Would you happen to have an idea, or would you be able to help me with that?

That failure is unrelated to your changes and either due to an update in pandas or an update in uv. I very much suspect it's the latter as I remember reading something about the default resolution strategy changing.
I will have a look later.

burgholzer · 2024-12-16T16:14:26Z

I am also trying to understand why one CI build fails, but I don't see how my changes produced this error. Would you happen to have an idea, or would you be able to help me with that?

That failure is unrelated to your changes and either due to an update in pandas or an update in uv. I very much suspect it's the latter as I remember reading something about the default resolution strategy changing. I will have a look later.

Turns out it's something else entirely. It's due to ubuntu-latest switching from ubuntu-22.04 to ubuntu-24.04. That change comes with a new gcc version and that version doesn't seem to be particularly happy with building pandas from source. I'll quickly fix this up. pandas ships 3.13 wheels as of v2.2.3.

Edit: should be fixed with 34d57b2 in #769

Joshy-R · 2024-12-16T17:49:08Z

Okay, thanks. Then, I will rebase onto the new main so that the test passes again. Do you have any further change requests regarding my pull request?

burgholzer · 2024-12-16T18:18:05Z

Okay, thanks. Then, I will rebase onto the new main so that the test passes again. Do you have any further change requests regarding my pull request?

Sounds good.
I am on it ☝️ I might have one or two more requests.

burgholzer

Many thanks again for your work on this!
I really like the way this is going and how it simplifies certain aspects in the library.
I feel we are really close to the finish line here. There are a couple of errors that snuck their way into the last couple of commits and some further suggestions for getting the implementation even cleaner and more performant.
I am fairly optimistic that this will be the last round of iteration.

burgholzer · 2024-12-16T17:54:01Z

include/mqt-core/dd/Operations.hpp

+    // some operations need parameter changes even in the non-inverse case
+    if (type == qc::U) {
+      assert(params.size() == 3U);
+      // shuffle [a, b, c] to [c, b, a]
+      std::reverse(params.begin(), params.end());
+    } else if (type == qc::U2) {
+      params[0U] = params[1U];
+    }


Wouldn't it be simpler to eliminate these special cases by refactoring the respective u2 and u matrix methods so that they take their parameters in the opposite order? Sounds like less special handling to me.

Furthermore, the U2 condition isn't quite right as it does not swap the parameters.

burgholzer · 2024-12-16T17:59:01Z

include/mqt-core/dd/Operations.hpp

+  auto type = op.getType();
+  std::vector<fp> params = op.getParameter();
+  std::vector<qc::Qubit> targetQubits = targets;


Sorry for being so picky here, but I believe this could be improved further by a bit.
At the moment, this incurs an extra copy for these variables in the inverse=false case that should not really be necessary.
Given that these copies are small, I would be fine with leaving it, but I wanted to bring it up in case you see an elegant solution that avoids that.

One solution that I could think of is to create a separate GetStandardOperationInverseDD method that essentially forwards to the getStandardOperationDD method after performing the necessary adaptations.
That way, copies should only be required where they are really necessary.
Instead of break statements in the respective case statements of the new inverse method, it could directly return the respective value from getStandardOperationDD, i.e., the self-inverse gates would return without any modifications, and the others would just construct a new StandardOperation with the right parameters and pass it over.

Yes, good point. I think we can solve this without a separate function and just have it inline like this:

if (not inverse) { // return gate directly } // construct inverse gate std::vector<qc::Qubit> targetQubits = targets; switch { // ... } // return inverse gate

This would have a duplicate call to gate creations, but as this is only two lines, I think this is ok.

That works for me 👍🏼 sounds like a nice compromise.

burgholzer · 2024-12-16T17:59:47Z

include/mqt-core/dd/Operations.hpp

+      params[0U] = -params[1U] + PI;
+      params[1U] = -params[1U] - PI;


This is not quite right as it does not preserve the old semantics

I will take a closer look at this

burgholzer · 2024-12-16T19:20:20Z

include/mqt-core/dd/Operations.hpp

@@ -186,17 +141,17 @@ getStandardOperationDD(const qc::StandardOperation* op, Package<Config>& dd,
 // An empty permutation marks the identity permutation.

 template <class Config>
-qc::MatrixDD getDD(const qc::Operation* op, Package<Config>& dd,
+qc::MatrixDD getDD(const qc::Operation& op, Package<Config>& dd,


Just a comment: I was a bit hesitant about this change at first, as I thought it would cause object slicing, but I convinced myself that this is not the case and this still works as intended. I agree with your assessment that this even makes the intent clearer because op == nullptr was never even handled in the first place.

burgholzer · 2024-12-16T19:22:33Z

include/mqt-core/dd/Operations.hpp

@@ -186,17 +141,17 @@ getStandardOperationDD(const qc::StandardOperation* op, Package<Config>& dd,
 // An empty permutation marks the identity permutation.

 template <class Config>
-qc::MatrixDD getDD(const qc::Operation* op, Package<Config>& dd,
+qc::MatrixDD getDD(const qc::Operation& op, Package<Config>& dd,


Given the comment above, I am also thinking whether it wouldn't be cleaner to separate the inverse function from the non-inverse function here.
I think the duplication would be at a minimum and the code would be easier to read overall.
What do you think?

burgholzer · 2024-12-16T19:23:16Z

include/mqt-core/dd/Operations.hpp

    if (qc::isTwoQubitGate(type)) {
      assert(targets.size() == 2);


I think this distinction on whether it is a one or two target gate is superfluous based on your changes in previous commits.

burgholzer · 2024-12-16T19:28:25Z

src/dd/GateMatrixDefinitions.cpp

+    assert(false && "Invalid single-qubit gate type");
+    return {}; // unreachable


If you really want to mark this as unreachable, then you can use qc::unreachable().

Suggested change

assert(false && "Invalid single-qubit gate type");

return {}; // unreachable

assert(false && "Invalid single-qubit gate type");

qc::unreachable();

Although that part of the code is technically reachable as the method is public and can easily be passed a wrong OpType.
I'd still be fine with adding the unreachable there (and down below).

If this code can get called by a library user, would it be better to raise an exception?

Most likely, yes.
Could be an std::invalid_argument exception.
That way, such errors get a proper error message even in Release mode.

burgholzer · 2024-12-16T19:31:24Z

test/dd/test_package.cpp

+  auto swapGate = dd->makeTwoQubitGateDD(dd::opToTwoQubitGateMatrix(qc::SWAP),
+                                         qc::Controls{}, 0, 2);


You should be able to also replace this with a corresponding StandardOperation call to stay consistent. (same goes for a couple of occurences down below)

burgholzer · 2024-12-16T19:34:18Z

test/dd/test_package.cpp

-TEST(DDPackageTest, TwoQubitControlledGateDDConstruction) {
-  const auto nrQubits = 5U;
-  const auto dd = std::make_unique<dd::Package<>>(nrQubits);
-
-  const auto gateMatrices = std::vector{std::pair{dd::X_MAT, dd::CX_MAT},
-                                        std::pair{dd::Z_MAT, dd::CZ_MAT}};
-
-  // For every combination of control and target, test that the DD created by
-  // makeTwoQubitGateDD is equal to the DD created by makeGateDD. This should
-  // cover every scenario of the makeTwoQubitGateDD function.
-  for (const auto& [gateMatrix, controlledGateMatrix] : gateMatrices) {
-    for (dd::Qubit control = 0; control < nrQubits; ++control) {
-      for (dd::Qubit target = 0; target < nrQubits; ++target) {
-        if (control == target) {
-          continue;
-        }
-        const auto controlledGateDD =
-            dd->makeTwoQubitGateDD(controlledGateMatrix, control, target);
-        const auto gateDD = dd->makeGateDD(
-            gateMatrix, qc::Control{static_cast<qc::Qubit>(control)}, target);
-        EXPECT_EQ(controlledGateDD, gateDD);
-      }
-    }
-  }
-}
-
-TEST(DDPackageTest, TwoQubitControlledGateDDConstructionNegativeControls) {
-  const auto nrQubits = 5U;
-  const auto dd = std::make_unique<dd::Package<>>(nrQubits);
-
-  const auto gateMatrices = std::vector{std::pair{dd::X_MAT, dd::CX_MAT},
-                                        std::pair{dd::Z_MAT, dd::CZ_MAT}};
-
-  // For every combination of controls, control type, and target, test that the
-  // DD created by makeTwoQubitGateDD is equal to the DD created by makeGateDD.
-  // This should cover every scenario of the makeTwoQubitGateDD function.
-  for (const auto& [gateMatrix, controlledGateMatrix] : gateMatrices) {
-    for (dd::Qubit control0 = 0; control0 < nrQubits; ++control0) {
-      for (dd::Qubit control1 = 0; control1 < nrQubits; ++control1) {
-        if (control0 == control1) {
-          continue;
-        }
-        for (dd::Qubit target = 0; target < nrQubits; ++target) {
-          if (control0 == target || control1 == target) {
-            continue;
-          }
-          for (const auto controlType :
-               {qc::Control::Type::Pos, qc::Control::Type::Neg}) {
-            const auto controlledGateDD = dd->makeTwoQubitGateDD(
-                controlledGateMatrix, qc::Controls{{control0, controlType}},
-                control1, target);
-            const auto gateDD = dd->makeGateDD(
-                gateMatrix, qc::Controls{{control0, controlType}, control1},
-                target);
-            EXPECT_EQ(controlledGateDD, gateDD);
-          }
-        }
-      }
-    }
-  }
-}


I'd be in favor of leaving these tests in, but just locally defining the CX and CZ matrices. These are good sanity checks for the DD generation routines.

Joshy-R force-pushed the refactor_handling_of_gate_matrices_and_inverses branch 2 times, most recently from 0c22ab7 to 3d824d7 Compare November 14, 2024 11:28

github-advanced-security bot found potential problems Nov 14, 2024

View reviewed changes

include/mqt-core/dd/Operations.hpp Fixed Show fixed Hide fixed

burgholzer added refactor Anything related to code refactoring Core Anything related to the Core library and IR c++ Anything related to C++ code labels Nov 14, 2024

burgholzer approved these changes Nov 14, 2024

View reviewed changes

burgholzer requested changes Nov 14, 2024

View reviewed changes

Joshy-R force-pushed the refactor_handling_of_gate_matrices_and_inverses branch 4 times, most recently from 4632650 to 165f26b Compare December 7, 2024 13:36

Joshy-R requested a review from burgholzer December 8, 2024 09:39

burgholzer requested changes Dec 11, 2024

View reviewed changes

Joshy-R added 8 commits December 15, 2024 22:58

♻️ move OpTypes to optype.inc

fd59d50

♻️ getamatrixdefinition: move matrix definitions as map to cpp file

9b5f48e

♻️ operations: simplify getStandardOperationDD

4001e82

♻️ 🔥 use opToSingleGateMatrix to retrieve matrix and remove unused CX…

74117b4

…_MAT and CZ_MAT

♻️ refractor dd operations

792abc6

refractor functions to receive Operation as const referece instead of pointer refractor getStandardOperationDD to receive targets as vector, remove overload that takes two targets and inline helper function as it now only has a single use

♻️ retrieve DDs over standardoperations in tests

db945c8

♻️ rename opToSingleGateMatrix to opToSingleQubitGateMatrix

d678b71

♻️ move helpers in GateMatrixDefinition from .hpp to .cpp

337ba29

Joshy-R force-pushed the refactor_handling_of_gate_matrices_and_inverses branch from 165f26b to 337ba29 Compare December 15, 2024 22:52

burgholzer requested changes Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor handling of gate matrices and inverses #752

Refactor handling of gate matrices and inverses #752

Joshy-R commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading

burgholzer left a comment

burgholzer left a comment

burgholzer left a comment

Joshy-R commented Dec 15, 2024

Joshy-R commented Dec 16, 2024

burgholzer commented Dec 16, 2024

burgholzer commented Dec 16, 2024 •

edited

Loading

Joshy-R commented Dec 16, 2024

burgholzer commented Dec 16, 2024

burgholzer left a comment

burgholzer Dec 16, 2024

burgholzer Dec 16, 2024

Joshy-R Dec 17, 2024

burgholzer Dec 17, 2024

burgholzer Dec 16, 2024

Joshy-R Dec 17, 2024

burgholzer Dec 16, 2024

burgholzer Dec 16, 2024

burgholzer Dec 16, 2024

burgholzer Dec 16, 2024

Joshy-R Dec 17, 2024

burgholzer Dec 17, 2024

burgholzer Dec 16, 2024

burgholzer Dec 16, 2024

		params[0U] = -params[1U] + PI;
		params[1U] = -params[1U] - PI;

		assert(false && "Invalid single-qubit gate type");
		return {}; // unreachable

		auto swapGate = dd->makeTwoQubitGateDD(dd::opToTwoQubitGateMatrix(qc::SWAP),
		qc::Controls{}, 0, 2);

Refactor handling of gate matrices and inverses #752

Are you sure you want to change the base?

Refactor handling of gate matrices and inverses #752

Conversation

Joshy-R commented Nov 14, 2024 • edited Loading

Description

Checklist:

codecov bot commented Nov 14, 2024 • edited Loading

Codecov Report

burgholzer left a comment

Choose a reason for hiding this comment

burgholzer left a comment

Choose a reason for hiding this comment

burgholzer left a comment

Choose a reason for hiding this comment

Joshy-R commented Dec 15, 2024

Joshy-R commented Dec 16, 2024

burgholzer commented Dec 16, 2024

burgholzer commented Dec 16, 2024 • edited Loading

Joshy-R commented Dec 16, 2024

burgholzer commented Dec 16, 2024

burgholzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joshy-R commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading

burgholzer commented Dec 16, 2024 •

edited

Loading