[refactor] new SingleQubitSquash class for squashing #168

lmondada · 2022-01-12T18:31:45Z

This refactor separates single-qubit squashing into two independent component:

the squashing logic (ie given a chain of single-qubit gates, what it should squash to)
the piping: how to pass through the circuit and substitute circuits

In so doing, it merges most of the implementations of standard_squash and pqp_squash into one, where before both where independent (with a lot of deduplication). This should allow us to add more squashing methods easily in the future.

As an added bonus, smart squashing now supports conditional operations!

lmondada · 2022-01-14T12:04:09Z

The commit history isn't particularly tidy or relevant, so you probably want to review everything in one block.

cqc-alec

Since this refactor adds more lines (865) than it removes (388), I assume the main motivation is that it will save adding new code in the GlobalisePhasedX pass?

Also nice that we can drop the NoClassicalControlPredicate in the squashing pass. Are you confident there is enough testing of squashing with classical control?

cqc-alec · 2022-01-17T09:37:34Z

tket/tests/test_CompilerPass.cpp

+    const Circuit& c = cu.get_circ_ref();
+    c.assert_valid();
+    REQUIRE(c.n_gates() == 3);
+    auto cmds = c.get_commands();


This is unused.

Yes, the squash_between method is making my life a lot easier. Although I think most of the code size increase is from comments, classes definition and white space -- the code before was very terse and there definitely isn't a 2x blow-up in the amount of code.

I'll add some testing.

cqc-alec · 2022-01-17T09:42:45Z

tket/src/Transformations/BasicOptimisation.cpp

@@ -32,8 +32,6 @@ static bool remove_redundancy(
    Circuit &circ, const Vertex &vert, VertexList &bin,
    std::set<IVertex> &new_affected_verts, IndexMap &im);
 static bool commute_singles_to_front(Circuit &circ);
-static bool squash_to_pqp(


Since you've made a new subdirectory for single-qubit squashing code we should probably also move Transform::squash_1qb_to_tk1() there.

cqc-alec · 2022-01-17T10:00:22Z

tket/src/Utils/Expression.cpp

-  return SymEngine::cos(e * SymEngine::pi / 2);
+  if (is_simpler(e + 2, e)) {
+    // use cos(x + pi) = -cos(x)
+    return SymEngine::mul(


I'm surprised symengine doesn't do this simplification. The machinery for it must be there -- see e.g. https://github.com/symengine/symengine/blob/5c47488c5d1df382d36959b0dad14916dd745f54/symengine/tests/basic/test_functions.cpp#L406

It works if I SymEngine::expand before the cos computation. Would you prefer that?

As in just return SymEngine::cos(Symengine::expand(e * SymEngine::pi / 2)); ? That seems better, yes!

cqc-alec · 2022-01-17T10:03:03Z

tket/src/Utils/Expression.cpp

@@ -92,12 +92,30 @@ static Expr cos_pi_by_12_times(double x) {
  }
 }

+static bool is_simpler(const Expr& e1, const Expr& e2) {


I worry a bit about the performance impact of this. At least we could save one call to expand (plus json conversion) and one call to dump by only evaluating them once for e (rather than repeating in the two calls to this function).

tket/src/Transformations/SingleQubitSquash/StandardSquash.cpp

cqc-alec · 2022-01-17T10:33:28Z

tket/src/Transformations/SingleQubitSquash/SingleQubitSquash.hpp

+    auto it1 = circ.begin();
+    auto it2 = gates.cbegin();
+
+    while (it1 != circ.end() && it2 != gates.end()) {


Can't we use Gate::operator== for most of this?

Absolutely!

cqc-alec · 2022-01-17T10:46:03Z

tket/src/Transformations/SingleQubitSquash/PQPSquash.cpp

+ *
+ * The squash is made backwards, so that rotations get pushed towards
+ * the front, see the design choice notes in confluence
+ * https://cqc.atlassian.net/l/c/17xm5hvp


CQC confluence isn't public so let's not link to it here. Instead summarize the reasons here if possible.

cqc-alec · 2022-01-17T10:47:32Z

tket/src/Transformations/SingleQubitSquash/PQPSquash.cpp

+          "Requires two different bases to perform single qubit "
+          "rotations");


Suggested change

"Requires two different bases to perform single qubit "

"rotations");

"Requires two different bases to perform single qubit rotations");

cqc-alec · 2022-01-17T10:57:02Z

tket/src/Transformations/SingleQubitSquash/SingleQubitSquash.hpp

+  }
+
+  // squash everything between edges in and out
+  bool squash_between(const Edge &in, const Edge &out) {


There is a lot of implementation in this header file. Is it worth moving some it out into non-templated "helper" source files, or does it mostly need to be templated?

I'm refactoring it to use polymorphism instead of templating. This makes the abstract interface more explicit and allows me to move all the implementation into a separate cpp file

cqc-alec · 2022-01-17T11:08:19Z

tket/src/Transformations/SingleQubitSquash/PQPSquash.cpp

+    bool z_followed_by_measures = true;
+    for (port_t port = 0; port < kids.size() && z_followed_by_measures;
+         port++) {
+      if (circ.get_OpType_from_Vertex(kids[port]) == OpType::Measure) {


Looking at the test coverage report (which is generally very good), I see a few lines here that are never executed. Can we add one or two more tests to execute lines 298, 310 and 316?

This should be fixed.

Actually, these lines would only be used if a squasher returned circuits with OpTypes Measure and noop, which currently they aren't (and arguably never should).

This part of the code was just copied over from before. I would suggest leaving everything as is for now, but I can add a task to revisit and simplify that remove_redundancy code, as most of it is probably unnecessary.

Co-authored-by: Alec Edgington <[email protected]>

cqc-alec · 2022-01-19T13:11:06Z

Running the test program after building tket seems to take forever! (I reproduced this locally as well.)

cqc-alec

I had to cancel the build because it was stuck running test after building tket...

cqc-alec

Looks good, just a couple of pedantic corrections to the doxygen and one question.

tket/src/Transformations/include/Transformations/SingleQubitSquash.hpp

cqc-alec · 2022-01-20T13:32:53Z

tket/src/Transformations/SingleQubitSquash.cpp

+    const Circuit &sub, const std::vector<Gate_ptr> chain) const {
+  const unsigned n_gates = sub.n_gates();
+  return n_gates < chain.size() ||
+         (n_gates == chain.size() && !is_equal(sub, chain, reversed_));


Can this ever lead to infinite sequences of squashes where the chain changes each time (cycling between values but staying the same length)?

It all depends on the squasher. As long as you have a "normal form" which squashing leaves unchanged (which all well-behaved squasher should have) you'll be fine. In our case this is definitely the case. I can add a line in the docs saying that.

cqc-alec · 2022-01-20T13:39:33Z

The test failure in the coverage build is puzzling...

…uash.hpp Co-authored-by: Alec Edgington <[email protected]>

cqc-alec

Still failing :-(

This reverts commit c3a1133.

lmondada requested a review from cqc-alec January 12, 2022 18:32

lmondada force-pushed the refactor/squashing branch from 922b6fe to 30e82ff Compare January 12, 2022 18:34

Refactor squashing

257800a

lmondada force-pushed the refactor/squashing branch from 30e82ff to 257800a Compare January 12, 2022 18:37

lmondada added 7 commits January 13, 2022 09:20

add explicit casting

ecada8c

do not commute no-ops

479231a

changed squash_wire => squash_between

62d4821

allow classical control in gen_euler_pass

2a8d3a0

commuting through special case

b2cb5c8

fixup_angle is consistent in reverse mode

ed47c1b

improve symbolic cos

abe6628

cqc-alec requested changes Jan 17, 2022

View reviewed changes

lmondada and others added 9 commits January 19, 2022 08:32

Update tket/src/Transformations/SingleQubitSquash/StandardSquash.cpp

4032bc8

Co-authored-by: Alec Edgington <[email protected]>

added conditional tests

f77dd8e

moved squash_1qb_to_pqp

1c5d234

added doc

07fae18

addressed more comments

5a6a1e5

refactor squasher interface

a8eb8e4

improved coverage

69e3293

fixed tests

6d3ffd3

Merge branch 'develop' into refactor/squashing

292774f

lmondada requested a review from cqc-alec January 19, 2022 11:43

added new files to local cmakefile

035125b

cqc-alec requested changes Jan 19, 2022

View reviewed changes

lmondada added 4 commits January 19, 2022 15:42

removed infinite loop

df1e3ae

removed two useless tests

6df0c8f

fixed test

36d814e

fixed test (2)

fee0ac0

replaced floats with fractions

75d19ad

lmondada force-pushed the refactor/squashing branch from 8ca3255 to 75d19ad Compare January 20, 2022 11:25

lmondada requested a review from cqc-alec January 20, 2022 11:26

cqc-alec reviewed Jan 20, 2022

View reviewed changes

lmondada and others added 3 commits January 20, 2022 13:45

Update tket/src/Transformations/include/Transformations/SingleQubitSq…

6fa5d90

…uash.hpp Co-authored-by: Alec Edgington <[email protected]>

Update tket/src/Transformations/include/Transformations/SingleQubitSq…

873f698

…uash.hpp Co-authored-by: Alec Edgington <[email protected]>

added comment on well-behaved squashes

ffa6bac

lmondada force-pushed the refactor/squashing branch from a45bf63 to ffa6bac Compare January 20, 2022 13:57

amended test instance

c3a1133

cqc-alec approved these changes Jan 20, 2022

View reviewed changes

cqc-alec requested changes Jan 20, 2022

View reviewed changes

cqc-alec added 2 commits January 24, 2022 13:46

Revert "amended test instance"

6344fc7

This reverts commit c3a1133.

Workaround for symengine issue.

2e06d5a

cqc-alec approved these changes Jan 24, 2022

View reviewed changes

cqc-alec merged commit 12478bd into develop Jan 24, 2022

cqc-alec deleted the refactor/squashing branch January 24, 2022 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] new SingleQubitSquash class for squashing #168

[refactor] new SingleQubitSquash class for squashing #168

lmondada commented Jan 12, 2022 •

edited

Loading

lmondada commented Jan 14, 2022

cqc-alec left a comment

cqc-alec Jan 17, 2022

lmondada Jan 19, 2022

cqc-alec Jan 17, 2022

lmondada Jan 19, 2022

cqc-alec Jan 17, 2022

lmondada Jan 19, 2022

cqc-alec Jan 19, 2022

cqc-alec Jan 17, 2022

cqc-alec Jan 17, 2022

lmondada Jan 19, 2022

cqc-alec Jan 17, 2022

lmondada Jan 19, 2022

cqc-alec Jan 17, 2022

cqc-alec Jan 17, 2022

lmondada Jan 19, 2022

cqc-alec Jan 17, 2022

lmondada Jan 19, 2022

lmondada Jan 19, 2022

cqc-alec commented Jan 19, 2022

cqc-alec left a comment

cqc-alec left a comment

cqc-alec Jan 20, 2022

lmondada Jan 20, 2022

cqc-alec commented Jan 20, 2022 •

edited

Loading

cqc-alec left a comment

		"Requires two different bases to perform single qubit "
		"rotations");

[refactor] new SingleQubitSquash class for squashing #168

[refactor] new SingleQubitSquash class for squashing #168

Conversation

lmondada commented Jan 12, 2022 • edited Loading

lmondada commented Jan 14, 2022

cqc-alec 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

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

cqc-alec commented Jan 19, 2022

cqc-alec left a comment

Choose a reason for hiding this comment

cqc-alec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cqc-alec commented Jan 20, 2022 • edited Loading

cqc-alec left a comment

Choose a reason for hiding this comment

lmondada commented Jan 12, 2022 •

edited

Loading

cqc-alec commented Jan 20, 2022 •

edited

Loading