Delayed Phase Correction #397

pehamTom · 2023-11-14T16:01:52Z

Description

This PR introduces a feature where stabilizer phases are ignored during Clifford synthesis. If that happens, the phases are corrected during a post-processing step.

Fixes #326

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.

For depth-optimality it is sufficient to impose that at least one gate has to be present on each qubit. The transition relation on the tableau itself constrains the gates enough already. For the maxsat solution some adjustments have to be made because it relies on the exactlyone constraint to be present.

…aulis

burgholzer · 2023-11-14T23:52:52Z

Hey @pehamTom 👋🏼

Haven't looked through this at all yet. Just a brief question: Is this something that's always guaranteed to be faster than the original version? If so, could we just fully replace the old version and avoid adding all those if statements for deciding between both options? Just thinking about maintainability here.

pehamTom · 2023-11-15T00:00:33Z

Ignoring the phase during synthesis does not guarantee depth-optimality. One could always back-propagate Paulis and fuse them with any single-qubit gates as a post-processing step. But I think it makes sense to respect the chosen gateset and leave the rest up to the user.

I wanted to compress the settings we have anyway. The maxsat method should be removed entirely for example as it is always worse. But that is something for another PR.

…aulis

codecov · 2023-11-15T15:24:50Z

Codecov Report

Merging #397 (4b48b48) into main (5453be3) will increase coverage by 0.1%.
Report is 2 commits behind head on main.
The diff coverage is 96.2%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #397     +/-   ##
=======================================
+ Coverage   92.3%   92.4%   +0.1%     
=======================================
  Files         44      49      +5     
  Lines       4158    4468    +310     
  Branches     701     764     +63     
=======================================
+ Hits        3839    4132    +293     
- Misses       319     336     +17

Flag	Coverage Δ
cpp	`92.0% <96.2%> (+0.2%)`	⬆️
python	`95.9% <ø> (ø)`

Files	Coverage Δ
include/cliffordsynthesis/Configuration.hpp	`100.0% <ø> (ø)`
include/cliffordsynthesis/Tableau.hpp	`97.6% <100.0%> (+0.5%)`	⬆️
...de/cliffordsynthesis/encoding/ObjectiveEncoder.hpp	`100.0% <100.0%> (ø)`
include/cliffordsynthesis/encoding/SATEncoder.hpp	`100.0% <ø> (ø)`
...e/cliffordsynthesis/encoding/SingleGateEncoder.hpp	`100.0% <ø> (ø)`
...lude/cliffordsynthesis/encoding/TableauEncoder.hpp	`100.0% <100.0%> (ø)`
src/Architecture.cpp	`95.0% <100.0%> (ø)`
src/cliffordsynthesis/CliffordSynthesizer.cpp	`97.5% <100.0%> (+<0.1%)`	⬆️
src/cliffordsynthesis/Utils.cpp	`100.0% <100.0%> (ø)`
...rc/cliffordsynthesis/encoding/MultiGateEncoder.cpp	`100.0% <100.0%> (ø)`
... and 10 more

... and 1 file with indirect coverage changes

burgholzer · 2023-11-16T16:42:22Z

Ignoring the phase during synthesis does not guarantee depth-optimality. One could always back-propagate Paulis and fuse them with any single-qubit gates as a post-processing step. But I think it makes sense to respect the chosen gateset and leave the rest up to the user.

I wanted to compress the settings we have anyway. The maxsat method should be removed entirely for example as it is always worse. But that is something for another PR.

Sorry for responding so late here. For some reason, I missed the notification for the message.

I see! In that case, the separate option definitely makes sense.
Compressing the settings is definitely much appreciated (in a separate PR, of course).

Is this PR ready for review then?

pehamTom · 2023-11-17T10:07:54Z

Now it is ready for review.

github-actions · 2023-11-17T10:27:24Z

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-tidy reports: 1 concern(s)

src/Architecture.cpp

/src/Architecture.cpp:95:21: warning: [readability-identifier-naming]

invalid case style for static constant 'singleQubitGates'

  static const auto singleQubitGates = {"id", "u1", "u2", "u3",
                    ^~~~~~~~~~~~~~~~
                    SINGLE_QUBIT_GATES

Have any feedback or feature suggestions? Share it here.

burgholzer

Many thanks for submitting this PR!

For future reference: It would have been appreciated if, instead of a large PR like this that accomplishes more than a single thing, this were a collection of smaller self-contained PRs. That would make it so much easier to reason about and would considerably speed up the adoption of changes in main.
I guess it's too late for that now in this case.
That's one of the main reasons why the list of comments below is rather long. Sorry for that.

You should be able to batch the smaller code suggestions together directly on GitHub and commit them in one go. That should get rid of some comments already.

Two things that I couldn't really annotate in the sources themselves:

Please update the PR description to reflect all the additions, optimisations, changes in this PR
Is this a feature that should be advertised in the documentation? For example in the existing Clifford Synthesis notebook?

burgholzer · 2023-11-17T18:16:20Z

extern/LogicBlocks

Please revert these unrelated submodule change

burgholzer · 2023-11-17T18:16:26Z

extern/mqt-core

Please revert these unrelated submodule change

burgholzer · 2023-11-17T18:17:58Z

include/cliffordsynthesis/Configuration.hpp

+  GateSet gateSet = {qc::OpType::None, qc::OpType::S, qc::OpType::Sdg,
+                     qc::OpType::H,    qc::OpType::X, qc::OpType::Y,
+                     qc::OpType::Z};


Maybe I am missing something, but I believe this is missing the SX and SXdg gates. (and potentially the I identity gate; although I guess that is not really necessary). Couldn't you just publicly expose the SINGLE_QUBIT_CLIFFORDS member from the GateSet class and use it here?

burgholzer · 2023-11-17T18:18:53Z

include/cliffordsynthesis/Configuration.hpp


 #include <plog/Log.h>
 #include <thread>
 #include <unordered_map>
 #include <variant>
+#include <vector>


Suggested change

#include <vector>

Seems unnecessary

burgholzer · 2023-11-17T18:21:16Z

include/cliffordsynthesis/GateSet.hpp

+  // Check if None is already contained and if not, append it
+  void appendNone() {
+    if (!containsGate(qc::OpType::None)) {
+      gates.push_back(qc::OpType::None);


Suggested change

gates.push_back(qc::OpType::None);

gates.emplace_back(qc::OpType::None);

burgholzer · 2023-11-17T20:18:01Z

src/python/bindings.cpp

All of the new options are missing from the Python stubs for QMAP. Please also add any additions to the bindings there.

burgholzer · 2023-11-17T20:20:06Z

test/cliffordsynthesis/test_synthesis.cpp

I am surprised by the lack of further tests asserting the proper behaviour given the size of the changes in this PR.
Is this really enough to ensure that this works as expected?
For example: you introduce single-qubit gate sets in this PR, but you newer test different gate-sets.
That seems very dangerous to me to introduce regressions.

burgholzer · 2023-11-17T20:27:30Z

src/cliffordsynthesis/encoding/PhaseCorrectionEncoder.cpp

+      iChange.emplace_back(logicbase::LogicTerm::ite(
+          paulis[q][0], LogicTerm(false), LogicTerm(false)));


Something seems off to me about that constraint no matter the value of the Paulis, this will always be false, right?
Like, this can be way simpler and outside the loop.

burgholzer · 2023-11-17T20:28:49Z

src/cliffordsynthesis/encoding/PhaseCorrectionEncoder.cpp

+void PhaseCorrectionEncoder::splitXorR(const LogicVector& changes) {
+  auto xorHelper = LogicVector{};
+  xorHelper.reserve(S);
+  for (std::size_t row = 0U; row < S; ++row) {
+    const std::string hName =
+        "h_" + std::to_string(row) + "_" + std::to_string(xorHelpers.size());
+    DEBUG() << "Creating helper variable for RChange XOR " << hName;
+    xorHelper.emplace_back(lb->makeVariable(hName));
+  }
+  xorHelpers.emplace_back(xorHelper);
+  if (xorHelpers.size() == 1) {
+    for (std::size_t row = 0U; row < S; ++row) {
+      lb->assertFormula(xorHelpers[0][row] == changes[row]);
+    }
+  } else {
+    for (std::size_t row = 0U; row < S; ++row) {
+      lb->assertFormula(
+          xorHelpers.back()[row] ==
+          (changes[row] != xorHelpers[xorHelpers.size() - 2][row]));
+    }
+  }
+}


This method is already implemented elsewhere if I am not mistaken. Can we avoid the code duplication here?

burgholzer · 2023-11-17T20:31:00Z

src/cliffordsynthesis/encoding/PhaseCorrectionEncoder.cpp

+    GateSet pauliGates{};
+    pauliGates.removePaulis();


This is a truly strange sequence of calls to me that only makes me ask: WHY? 🙃
You create an empty gate set and then you remove Paulis from that?

pehamTom and others added 24 commits November 10, 2023 11:25

Removed fixed gateset and replaced with parameter.

c81eb40

Check if gateset is Clifford.

5eb3747

Added completens check for single-qubit Gatesets.

8c9d539

Extracted Gateset in separate file and pass gateset through config.

1cd55fd

Bindings for Gateset.

256f588

Commit missing file.

d218f42

Always add the none gate.

eb35989

Merge branch 'tom-dev' into relax-exactly-one-constraint

b2e3a92

Removed templates.

848e9d7

Merge branch 'tom-dev' into relax-exactly-one-constraint

7690afa

Made GateSet into an actual set.

d334f58

Merge branch 'tom-dev' into relax-exactly-one-constraint

267c2b1

Can only relax constraints if no Paulis are in the target Gateset.

f58a738

added setting for delaying paulis.

3b3f028

Extract common functionality of different encoders.

e6c3348

Added new class for handling the encoding for phase correction.

f45ac80

Added files for encoder.

cad2a04

Encoding and tests for delayed phase correction.

c02c8ed

Fixed tests.

0608ae9

🎨 pre-commit fixes

e8d4794

Fix constexpr error.

b8423cc

Merge remote-tracking branch 'origin/postpone-paulis' into postpone-p…

995ed9d

…aulis

🎨 pre-commit fixes

6a3bafa

burgholzer assigned pehamTom Nov 14, 2023

burgholzer added feature New feature or request c++ Anything related to C++ code minor Changes leading to a minor version increase labels Nov 14, 2023

pehamTom and others added 6 commits November 15, 2023 10:57

missing header?

dac77bb

Merge remote-tracking branch 'origin/postpone-paulis' into postpone-p…

60f64ca

…aulis

Missing include.

4291548

🎨 pre-commit fixes

245f55c

Linter warnings.

a0652f0

Merge remote-tracking branch 'origin/postpone-paulis' into postpone-p…

aaeb22f

…aulis

pehamTom changed the base branch from tom-dev to main November 15, 2023 15:20

Fixed relation of consistency constraints.

4b48b48

pehamTom requested a review from burgholzer November 17, 2023 10:07

burgholzer marked this pull request as ready for review November 17, 2023 16:15

burgholzer requested changes Nov 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delayed Phase Correction #397

Delayed Phase Correction #397

pehamTom commented Nov 14, 2023

burgholzer commented Nov 14, 2023

pehamTom commented Nov 15, 2023 •

edited

Loading

codecov bot commented Nov 15, 2023 •

edited

Loading

burgholzer commented Nov 16, 2023

pehamTom commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

burgholzer left a comment

burgholzer Nov 17, 2023

burgholzer Nov 17, 2023

burgholzer Nov 17, 2023

burgholzer Nov 17, 2023

burgholzer Nov 17, 2023

burgholzer Nov 17, 2023

burgholzer Nov 17, 2023

burgholzer Nov 17, 2023

burgholzer Nov 17, 2023

burgholzer Nov 17, 2023

	gates.push_back(qc::OpType::None);
	gates.emplace_back(qc::OpType::None);

		iChange.emplace_back(logicbase::LogicTerm::ite(
		paulis[q][0], LogicTerm(false), LogicTerm(false)));

Delayed Phase Correction #397

Are you sure you want to change the base?

Delayed Phase Correction #397

Conversation

pehamTom commented Nov 14, 2023

Description

Checklist:

burgholzer commented Nov 14, 2023

pehamTom commented Nov 15, 2023 • edited Loading

codecov bot commented Nov 15, 2023 • edited Loading

Codecov Report

burgholzer commented Nov 16, 2023

pehamTom commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

Cpp-Linter Report ⚠️

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

pehamTom commented Nov 15, 2023 •

edited

Loading

codecov bot commented Nov 15, 2023 •

edited

Loading