Feature/TwoQubitEncoding #342

avelsh · 2023-07-08T13:37:59Z

Added functionality for SQG-TQG shape encoding

pehamTom

This already looks really good. I have given you some pointers in my review that should help identify problematic sections of the code.

In general, I would like to change the name from STQGatesEncoder to simply TwoQubitEncoder. In contrast to the MultiGateEncoder this should give an idea that this class is used for solving problems wrt. two-qubit gates.

pehamTom · 2023-07-11T05:57:13Z

include/cliffordsynthesis/encoding/GateEncoder.hpp

@@ -129,7 +129,7 @@ class GateEncoder {
  virtual void assertGateConstraints()                           = 0;
  virtual void assertSingleQubitGateConstraints(std::size_t pos) = 0;
  virtual void assertTwoQubitGateConstraints(std::size_t pos)    = 0;
-  [[nodiscard]] static std::vector<TransformationFamily>
+  [[nodiscard]] virtual std::vector<TransformationFamily>


I don't think this has to be virtual necessarily. You could shadow the definition from GateEncoder since the method is only used in the derived class.

pehamTom · 2023-07-11T06:00:44Z

src/cliffordsynthesis/encoding/SATEncoder.cpp

-                                config.initialTableau->hasDestabilizers()
-                            ? 2U * N
-                            : N;
+  std::size_t s = config.targetTableau->hasDestabilizers() &&


Is there a reason that this is not const anymore?

pehamTom · 2023-07-11T06:14:32Z

src/cliffordsynthesis/encoding/STQGatesEncoder.cpp

+  }
+}
+
+void encoding::STQGatesEncoder::createTwoQubitGateVariables() {


As it stands, much code is repeated from GateEncoder.cpp. You could call the base class method and then loop over the timesteps to insert the identity variables separately. Or don't overwrite the method and use g_i_i as the variable for the identity gate.

pehamTom · 2023-07-11T06:22:55Z

src/cliffordsynthesis/encoding/STQGatesEncoder.cpp

+  const auto  n             = twoQubitGates.size();
+  for (std::size_t q = 0; q < n; ++q) {
+    if (q == qubit) {
+      if (N > variables.size()) {


I think I don't get this check. You could check for the target flag. This way you would also only add the variable for the identity once.

pehamTom · 2023-07-11T06:28:09Z

src/cliffordsynthesis/encoding/STQGatesEncoder.cpp

+  for (std::size_t t = 0U; t < T; ++t) {
+    TRACE() << "Asserting gate constraints at time " << t;
+
+    const std::size_t pos = t < T / 2 ? t : t % (T / 2);


As the Linter says, this is a possible division by Zero if T is 0. Also, I think it would improve clarity to add a class member that is essentially just T/2 that is set in the constructor. Then T would be the number of tableaus and T/2 would be the number of layers.

pehamTom · 2023-07-11T06:35:01Z

src/cliffordsynthesis/encoding/STQGatesEncoder.cpp

+  for (std::size_t ctrl = 0U; ctrl < N; ++ctrl) {
+    for (std::size_t trgt = 0U; trgt < N; ++trgt) {
+      if (ctrl == trgt) {
+        continue;


There is a constraint that needs to be added here. You also have to assert that when the None gate is set in the two-qubit layer there are no changes. At the moment, when the solver decides to set the identity the tableau variables are unconstrained and can be anything.

pehamTom · 2023-07-11T06:39:10Z

src/cliffordsynthesis/encoding/STQGatesEncoder.cpp

+  }
+}
+
+LogicTerm encoding::STQGatesEncoder::createTwoQubitGateConstraint(


If I'm not missing something here, this method can be removed as it is just a duplicated from the base class.

pehamTom · 2023-07-11T06:46:12Z

include/cliffordsynthesis/encoding/STQGatesEncoder.hpp

+      const GateToTransformation& gateToTransformation) override;
+
+  void
+  extractSingleQubitGatesFromModel(std::size_t pos, logicbase::Model& model,


You need to overwrite the GateEncoder::extractCircuitFromModel method as well. At the moment this is

for (std::size_t t = 0; t < T; ++t) { extractSingleQubitGatesFromModel(t, model, qc, nSingleQubitGates); extractTwoQubitGatesFromModel(t, model, qc, nTwoQubitGates); }``` The variables are extracted for all `t` up to `T`. But the variables only make sense for even or odd `t,` respectively.

# Conflicts: # include/cliffordsynthesis/encoding/STQGatesEncoder.hpp # src/cliffordsynthesis/encoding/STQGatesEncoder.cpp

# Conflicts: # include/cliffordsynthesis/encoding/TwoQubitEncoder.hpp # src/cliffordsynthesis/encoding/SATEncoder.cpp # src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

+    assertZConstraints(pos, q);
+    assertXConstraints(pos, q);
+    if (q != pos) {
+      // assertRConstraints(pos, q);


src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

+    if (q != pos) {
+      // assertRConstraints(pos, q);
+    }


# Conflicts: # src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

+                : assertTwoQubitGateConstraints(pos);
+    splitXorR(tvars->r[pos], pos);
+    TRACE() << "Asserting r changes at time " << t;
+    // lb->assertFormula(tvars->r[pos + 1] == xorHelpers[pos].back());


# Conflicts: # src/cliffordsynthesis/CliffordSynthesizer.cpp # src/cliffordsynthesis/encoding/SATEncoder.cpp

include/cliffordsynthesis/encoding/GateEncoder.hpp

pehamTom

Thank you for your work. I have reviewed your code and pointed out changes I'd like to see.
As stated in the comments you will most certainly need another single-qubit gate layer after the last two-qubit gate layer.
The naming TQDepth is not very self-explainatory. Please rename the respective variables to twoQubitDepth...

pehamTom · 2023-09-09T11:38:38Z

include/cliffordsynthesis/Configuration.hpp

@@ -38,6 +38,9 @@ struct Configuration {
  /// Settings for depth-optimal synthesis
  bool minimizeGatesAfterDepthOptimization = false;

+  /// Settings for STDepth-optimal synthesis


Comment inconsistent with variable naming

pehamTom · 2023-09-09T11:38:38Z

include/cliffordsynthesis/Results.hpp

@@ -40,6 +40,7 @@ class Results {
    return singleQubitGates;
  }
  [[nodiscard]] std::size_t       getDepth() const { return depth; }
+  [[nodiscard]] std::size_t       getTQDepth() const { return tQDepth; }


please rename

pehamTom · 2023-09-09T11:38:38Z

include/cliffordsynthesis/Results.hpp

@@ -52,6 +53,7 @@
  void setSingleQubitGates(const std::size_t g) { singleQubitGates = g; }
  void setTwoQubitGates(const std::size_t g) { twoQubitGates = g; }
  void setDepth(const std::size_t d) { depth = d; }
+  void setTQDepth(const std::size_t d) { tQDepth = d; }


please rename

pehamTom · 2023-09-09T11:38:38Z

include/cliffordsynthesis/Results.hpp

@@ -80,6 +82,7 @@
    resultJSON["single_qubit_gates"] = singleQubitGates;
    resultJSON["two_qubit_gates"]    = twoQubitGates;
    resultJSON["depth"]              = depth;
+    resultJSON["tQDepth"]            = tQDepth;


please rename

pehamTom · 2023-09-09T11:38:38Z

include/cliffordsynthesis/Results.hpp

@@ -96,8 +99,10 @@
  std::size_t       singleQubitGates = std::numeric_limits<std::size_t>::max();
  std::size_t       twoQubitGates    = std::numeric_limits<std::size_t>::max();
  std::size_t       depth            = std::numeric_limits<std::size_t>::max();
-  double            runtime          = 0.0;
-  std::size_t       solverCalls      = 0U;
+  std::size_t       tQDepth          = std::numeric_limits<std::size_t>::max();


please rename

pehamTom · 2023-09-09T11:38:40Z

src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

+  }
+}
+
+void encoding::TwoQubitEncoder::assertGateConstraints() {


After playing around with the code a bit I am now 100% convinced that a last single-qubit gate layer is need after the last two-qubit gate layer. Otherwise depth-optimality cannot be guaranteed.

pehamTom · 2023-09-09T11:38:40Z

src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

+    TRACE() << "Asserting gate constraints at time " << t;
+    splitXorR(tvars->r[t], t);
+    if (t == T) {
+      assertPauliGateConstraints(t);


Again, this doesn't need to be in the loop.

pehamTom · 2023-09-09T11:38:40Z

src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

+      splitXorR(change, pos);
+    }
+    lb->assertFormula(tvars->x[pos][q] == tvars->x[pos + 1][q]);
+    lb->assertFormula(tvars->z[pos][q] == tvars->z[pos + 1][q]);


Not necessary, but since only the phase is adjusted here, one could save on Tableau Variables.

pehamTom · 2023-09-09T11:38:40Z

src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

+
+LogicTerm
+encoding::TwoQubitEncoder::createIdentityConstraintOnTQG(std::size_t pos,
+                                                         std::size_t ctrl) {


This does not have to be a method. It is simple enough to be inlined and is only called once.

pehamTom · 2023-09-09T11:38:40Z

src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

+
+  res.setSingleQubitGates(nSingleQubitGates);
+  res.setTwoQubitGates(nTwoQubitGates);
+  res.setTQDepth(qc.getTQDepth());


This should be for the other encodings as well so you can compare against them. Add this computation to GateEncoder::extractCircuitFromModel

# Conflicts: # include/cliffordsynthesis/encoding/GateEncoder.hpp # src/cliffordsynthesis/encoding/GateEncoder.cpp # src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp # test/cliffordsynthesis/test_synthesis.cpp

# Conflicts: # src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

Aleksei Velsh and others added 5 commits June 23, 2023 18:42

set the main structure

5a32776

Merge branch 'main' into feature/avelsh

014f075

🎨 pre-commit fixes

9c9256e

add SQG and TQG encoding

a7822e0

🎨 pre-commit fixes

59393a8

pehamTom reviewed Jul 11, 2023

View reviewed changes

pehamTom marked this pull request as draft July 11, 2023 06:51

pehamTom assigned pehamTom and unassigned pehamTom Jul 11, 2023

pehamTom added feature New feature or request c++ Anything related to C++ code enhancement Anything related to improvements of the existing library minor Changes leading to a minor version increase labels Jul 11, 2023

Aleksei Velsh and others added 6 commits July 13, 2023 16:18

update comments

cdd566e

Merge remote-tracking branch 'origin/feature/avelsh' into feature/avelsh

716b8a6

# Conflicts: # include/cliffordsynthesis/encoding/STQGatesEncoder.hpp # src/cliffordsynthesis/encoding/STQGatesEncoder.cpp

🎨 pre-commit fixes

499b06e

update according to comments, rename two qubit encoder

d283ef8

Merge remote-tracking branch 'origin/feature/avelsh' into feature/avelsh

77cc2a5

# Conflicts: # include/cliffordsynthesis/encoding/TwoQubitEncoder.hpp # src/cliffordsynthesis/encoding/SATEncoder.cpp # src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

🎨 pre-commit fixes

7c7d542

github-advanced-security bot found potential problems Jul 30, 2023

View reviewed changes

Aleksei Velsh and others added 3 commits August 9, 2023 13:39

correct asssertTwoQubitGateConstr

9b0155a

Merge remote-tracking branch 'origin/feature/avelsh' into feature/avelsh

c9c1a44

# Conflicts: # src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

🎨 pre-commit fixes

0266b24

github-advanced-security bot found potential problems Aug 9, 2023

View reviewed changes

Aleksei Velsh and others added 6 commits August 14, 2023 15:33

fix two qubit encoding

122dd5d

rename from STDepth to TQDepth

1c0a309

sync with upstream

7ca3ec7

🎨 pre-commit fixes

dfea58a

remove addIdentityGateToTQGVariables func

cf12bef

set a new func for founding of a two qubit depth

c095997

Aleksei Velsh and others added 5 commits August 16, 2023 12:39

Merge remote-tracking branch 'origin/feature/avelsh' into feature/avelsh

b6e11dd

# Conflicts: # src/cliffordsynthesis/CliffordSynthesizer.cpp # src/cliffordsynthesis/encoding/SATEncoder.cpp

🎨 pre-commit fixes

b128086

fix founding of phase and set the last layer of paulis

e37653d

remove irrelevant tests

8d1da8f

set bindings and small fixes

d042b11

avelsh changed the title ~~Feature/s_tqg_encoding~~ Feature/TwoQubitEncoding Sep 6, 2023

Aleksei Velsh and others added 4 commits September 6, 2023 20:42

optimize the using of variables

92c5df6

Merge branch 'main' into feature/avelsh

727fba8

Merge remote-tracking branch 'avelsh/feature/avelsh' into feature/avelsh

0425bfc

🎨 pre-commit fixes

bd9af05

github-advanced-security bot found potential problems Sep 9, 2023

View reviewed changes

include/cliffordsynthesis/encoding/GateEncoder.hpp Fixed Show fixed Hide fixed

Added parsing of target metric for two_qubit_depth

3a4c206

pehamTom reviewed Sep 9, 2023

View reviewed changes

pehamTom and others added 8 commits September 9, 2023 13:40

Merge remote-tracking branch 'avelsh/feature/avelsh' into feature/avelsh

247e276

update according comments

1b39f2b

Merge remote-tracking branch 'origin/feature/avelsh' into feature/avelsh

c8bb1d6

# Conflicts: # include/cliffordsynthesis/encoding/GateEncoder.hpp # src/cliffordsynthesis/encoding/GateEncoder.cpp # src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp # test/cliffordsynthesis/test_synthesis.cpp

🎨 pre-commit fixes

a80fd55

set a new extra SQG layer

098d4d6

🎨 pre-commit fixes

caafb46

create a new func pauliGateToIndex

31090be

Merge remote-tracking branch 'origin/feature/avelsh' into feature/avelsh

292052b

# Conflicts: # src/cliffordsynthesis/encoding/TwoQubitEncoder.cpp

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/TwoQubitEncoding #342

Feature/TwoQubitEncoding #342

avelsh commented Jul 8, 2023

pehamTom left a comment

pehamTom Jul 11, 2023

pehamTom Jul 11, 2023

pehamTom Jul 11, 2023

pehamTom Jul 11, 2023

pehamTom Jul 11, 2023

pehamTom Jul 11, 2023

pehamTom Jul 11, 2023

pehamTom Jul 11, 2023

pehamTom left a comment

pehamTom Sep 9, 2023

pehamTom Sep 9, 2023

pehamTom Sep 9, 2023

pehamTom Sep 9, 2023

pehamTom Sep 9, 2023

pehamTom Sep 9, 2023

pehamTom Sep 9, 2023

pehamTom Sep 9, 2023

pehamTom Sep 9, 2023

pehamTom Sep 9, 2023

Feature/TwoQubitEncoding #342

Are you sure you want to change the base?

Feature/TwoQubitEncoding #342

Conversation

avelsh commented Jul 8, 2023

pehamTom 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

pehamTom 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