Avoid exploring extraneous minima in the cut-finder search space #585

ibrahim-shehzad · 2024-05-10T06:03:38Z

A certain step in the cut-finder explores multiple minima of the cost function. This step, however, is only relevant when we have LOCC or LO blackbox/parallel gate cut QPD assignments. As such, it is unnecessary for the LO circuit cutting that we currently support. Disabling the search for multiple minima also helps speed up the performance of the cut finder. This was especially relevant for certain QAOA circuits that were reported to us, which involved multiple Rzz gates with $\theta =0$ (in which case the cost of cutting each of these gates is 1). Telling the cut finder to stop at the first minimum stops it from exploring an evergrowing list of states (since no pruning of states can take place when the cost of each additional gate cut just gives us a multiplicative factor of 1 to the overall cost). This PR aims to fix all of this, simply by setting the default value of certain stop_at_first_min flags to True.

coveralls · 2024-05-10T06:47:07Z

Pull Request Test Coverage Report for Build 9083375618

Details

13 of 13 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.004%) to 95.497%

Totals
Change from base Build 9021914548:	0.004%
Covered Lines:	3499
Relevant Lines:	3664

💛 - Coveralls

garrison · 2024-05-10T11:02:56Z

circuit_knitting/cutting/cut_finding/best_first_search.py

@@ -149,6 +149,8 @@ class BestFirstSearch:

    ``stop_at_first_min`` (Boolean) is a flag that indicates whether or not to
    stop the search after the first minimum-cost goal state has been reached.
+    In the absence of any QPD assignments, it always makes sense to stop once


What does "qpd assignments"mean?

Thanks for raising this. So the way Ed's overall code was designed was that in the first stage, it tries to find locations for all the cuts (assuming everything is just LO, since that provides a cost (overhead) upperbound) and then, in the second stage, assigns the actual QPD's (where the user can chose if it's instead LOCC, blackbox LO, etc.). The idea behind giving the user this choice was that being able to pick between LO, LOCC, and blackbox LO etc. can depend on the resources that are available (e.g whether or not CC is possible and whether or not you have access to ancillas etc). One should also in principle be able to make a different choice for each cut. In this situation, not all sets of cuts found in the first step are created equal; some may be more suitable than others.

Now when you're only even allowing for ''vanilla'' LO, all you are ever doing is the first step and so there is no need to explore multiple minima.

In summary, what I meant by "QPD assignments" was any non-LO QPD assignments.

circuit_knitting/cutting/cut_finding/best_first_search.py

garrison · 2024-05-10T18:37:04Z

circuit_knitting/cutting/cut_finding/best_first_search.py

@@ -268,7 +269,7 @@ def optimization_pass(

            self.update_minimum_reached(cost)

-            if cost is None or self.cost_bounds_exceeded(cost):
+            if cost is None or self.cost_bounds_exceeded(cost):  # pragma: no cover


Do you understand why this change resulted in this branch losing coverage?

I think in exploring the multiple minima, it was encountering a None state for one of the test cases. I'd have to look into this more to figure out exactly what was going on.

I have added a test for this case (here: c4c2968). The test also allows you to describe the workflow of the optimizer at a more granular level. We force BestFirstSearch.optimize() to run until it encounters a None state which then tests this line. I am a little concerned, though, that I overdid this one a little bit and maybe excluding this line from coverage instead would have been okay.

I think it's a good test, thanks

Co-authored-by: Jim Garrison <[email protected]>

garrison

Thanks, looks reasonable to me. 🚀

caleb-johnson · 2024-05-10T21:38:37Z

Could do a release note if we think the cut-finding performance is noticeable enough in general usage.

caleb-johnson · 2024-05-10T21:40:50Z

test/cutting/cut_finding/test_cut_finder_results.py

@@ -190,7 +190,7 @@ def test_four_qubit_circuit_two_qubit_qpu(
    )  # circuit separated into 2 subcircuits.

    assert (
-        optimization_pass.get_stats()["CutOptimization"] == array([15, 46, 15, 6])
+        optimization_pass.get_stats()["CutOptimization"] == array([11, 36, 15, 4])


Is there some sense I can make of these indices? (I think they're indices). How do you know what they should be here?

They're basically just stats that keep track of things like how many states were added to the queue and how many backjumps were performed during the search. These numbers were obtained just by running the search algorithm on these circuits.

OK it's dubious to let the output of your function determine the target values in your unit tests. It's better to manually track down whether these values are correct when making the test case (even if it's just printing a bunch of things out and sanity checking for yourself). I understand the temptation in situations like these, but doing this can give you a false sense of security and defeat the purpose of unit testing.

Once you've satisfied yourself that these values are actually what they should be, you can normally just let the other devs know in code review that you verified this output. They can verify it themselves if they choose.

I think it was partly also because I was not able to come up with a more creative test for this function. I guess for a small enough circuit though, one may be able to predict some of these numbers.

I would probably remove the test if it were my code, since there could very well be a bug here that is treated as ground truth just because that was the output. No one has actually checked that these are the values that should've been returned for this given circuit.

It's kind of outside the scope of this PR, but I just wanted to give my $.02 on that :D.

No that's actually a good point. I am going to change this test.

caleb-johnson

Awesome LGTM 🚀

I'll let you decide whether you think a performance release note is worthy here

…Qiskit-Extensions/circuit-knitting-toolbox into avoid-exploring-multiple-minima

garrison · 2024-05-13T20:24:40Z

test/cutting/cut_finding/test_cut_finder_results.py

-    assert (
-        optimization_pass.get_stats()["CutOptimization"] == array([15, 46, 15, 6])
-    ).all()  # matches known stats.
+    assert optimization_pass.get_stats()["CutOptimization"][3] <= settings.max_backjumps


Why only consider the element at index [3]?

It's really the only number (the number of backjumps) that is possible to (read easiest to) predict and constrain.

I see now: get_stats returns an array but every element of it means something specific. This would have been better as a data structure or namedtuple rather than a numpy array.

'Tis done (837875c). Have also added a release note.

'Tis done (837875c). Have also added a release note.

Thank you.

We should probably actually revert this on this branch and then do this as a separate PR. That way, we can backport this current PR since the interface does not change. The improved interface can be done in 0.8.0 with a release note.

Hmm this function isn't actually exposed through the API. Does that matter?

Hmm this function isn't actually exposed through the API. Does that matter?

OK, in that case back-porting should be fine as is.

Sorry, I thought I saw something previously in the notebook where these same four numbers turned up, but that is either gone now or a phantom memory.

garrison · 2024-05-14T16:48:46Z

circuit_knitting/cutting/cut_finding/best_first_search.py

@@ -299,10 +314,10 @@ def minimum_reached(self) -> bool:
        """Return True if the optimization reached a global minimum."""
        return self.min_reached

-    def get_stats(self, penultimate: bool = False) -> np.typing.NDArray[np.int_] | None:
+    def get_stats(self, penultimate: bool = False) -> NamedTuple | None:


Does it complain if you use SearchStats as the return type?

It doesn't, I have changed that here (d931ec6).

circuit_knitting/cutting/cut_finding/best_first_search.py

Co-authored-by: Jim Garrison <[email protected]>

…Qiskit-Extensions/circuit-knitting-toolbox into avoid-exploring-multiple-minima

garrison · 2024-05-14T17:35:05Z

circuit_knitting/cutting/cut_finding/lo_cuts_optimizer.py

@@ -155,10 +152,10 @@ def get_results(self) -> DisjointSubcircuitsState | None:
        """Return the optimization results."""
        return self.best_result

-    def get_stats(self, penultimate=False) -> dict[str, NDArray[np.int_]]:
+    def get_stats(self, penultimate=False) -> dict[str, NamedTuple | None]:


Is the idea that there might one day be more keys in this dict than just CutOptimization?

I wonder if this would be better.

Suggested change

def get_stats(self, penultimate=False) -> dict[str, NamedTuple | None]:

def get_stats(self, penultimate=False) -> dict[str, Any]:

but the docstring is still a little bit weird, because it talks about the "value" of the dict without referencing what the key(s) are.

Is the idea that there might one day be more keys in this dict than just CutOptimization?

Yes, that's right.

In that case, I think my suggestion of Any makes the most sense, and maybe edit the docstring for clarity too.

garrison

I am happy with this

* Avoid exploring extraneous minima in the search space * fix failing test * fix coverage * black * update doc string * update doc string Co-authored-by: Jim Garrison <[email protected]> * add new tests and modify states check * update test description * style * change to namedtuple, add release note * update return Co-authored-by: Jim Garrison <[email protected]> * change type hints --------- Co-authored-by: Jim Garrison <[email protected]> (cherry picked from commit 2bcbe7f)

… (#588) * Avoid exploring extraneous minima in the search space * fix failing test * fix coverage * black * update doc string * update doc string Co-authored-by: Jim Garrison <[email protected]> * add new tests and modify states check * update test description * style * change to namedtuple, add release note * update return Co-authored-by: Jim Garrison <[email protected]> * change type hints --------- Co-authored-by: Jim Garrison <[email protected]> (cherry picked from commit 2bcbe7f) Co-authored-by: Ibrahim Shehzad <[email protected]>

Avoid exploring extraneous minima in the search space

5911b10

ibrahim-shehzad added bug Something isn't working enhancement New feature or request cut finder Related to the automatic cut finder labels May 10, 2024

ibrahim-shehzad requested review from garrison and caleb-johnson May 10, 2024 06:03

ibrahim-shehzad self-assigned this May 10, 2024

ibrahim-shehzad added 2 commits May 10, 2024 02:27

fix failing test

bbd5d11

fix coverage

59eab4f

black

16d2a13

ibrahim-shehzad changed the title ~~Avoid exploring extraneous minima in the search space~~ Avoid exploring extraneous minima in the cut-finder search space May 10, 2024

ibrahim-shehzad requested review from garrison and caleb-johnson and removed request for garrison and caleb-johnson May 10, 2024 07:02

garrison reviewed May 10, 2024

View reviewed changes

garrison added the stable backport potential Suitable to be backported to most recent stable branch by Mergify label May 10, 2024

garrison added this to the 0.7.2 milestone May 10, 2024

update doc string

580b4a9

garrison reviewed May 10, 2024

View reviewed changes

circuit_knitting/cutting/cut_finding/best_first_search.py Outdated Show resolved Hide resolved

garrison reviewed May 10, 2024

View reviewed changes

update doc string

5998d8f

Co-authored-by: Jim Garrison <[email protected]>

garrison previously approved these changes May 10, 2024

View reviewed changes

caleb-johnson reviewed May 10, 2024

View reviewed changes

caleb-johnson previously approved these changes May 12, 2024

View reviewed changes

ibrahim-shehzad added 2 commits May 13, 2024 10:23

add new tests and modify states check

c4c2968

Merge branch 'avoid-exploring-multiple-minima' of https://github.com/…

f2a8ad6

…Qiskit-Extensions/circuit-knitting-toolbox into avoid-exploring-multiple-minima

ibrahim-shehzad dismissed caleb-johnson’s stale review via f2a8ad6 May 13, 2024 14:23

ibrahim-shehzad dismissed garrison’s stale review via f2a8ad6 May 13, 2024 14:23

ibrahim-shehzad added 2 commits May 13, 2024 10:36

update test description

603af4a

style

16ecc0f

garrison reviewed May 13, 2024

View reviewed changes

change to namedtuple, add release note

837875c

ibrahim-shehzad requested a review from garrison May 14, 2024 16:42

garrison reviewed May 14, 2024

View reviewed changes

circuit_knitting/cutting/cut_finding/best_first_search.py Outdated Show resolved Hide resolved

ibrahim-shehzad and others added 3 commits May 14, 2024 13:03

update return

6ce2d5e

Co-authored-by: Jim Garrison <[email protected]>

change type hints

d931ec6

Merge branch 'avoid-exploring-multiple-minima' of https://github.com/…

d774c5b

…Qiskit-Extensions/circuit-knitting-toolbox into avoid-exploring-multiple-minima

garrison reviewed May 14, 2024

View reviewed changes

garrison approved these changes May 14, 2024

View reviewed changes

ibrahim-shehzad merged commit 2bcbe7f into main May 14, 2024
11 checks passed

ibrahim-shehzad deleted the avoid-exploring-multiple-minima branch May 14, 2024 19:02

mergify bot mentioned this pull request May 14, 2024

Avoid exploring extraneous minima in the cut-finder search space (backport #585) #588

Merged

garrison mentioned this pull request May 29, 2024

Provide options in the cut-finder API to turn LO gate and wire cut finding off or on, expose min-reached flag. #586

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid exploring extraneous minima in the cut-finder search space #585

Avoid exploring extraneous minima in the cut-finder search space #585

ibrahim-shehzad commented May 10, 2024 •

edited

Loading

coveralls commented May 10, 2024 •

edited

Loading

garrison May 10, 2024

ibrahim-shehzad May 10, 2024

garrison May 10, 2024

ibrahim-shehzad May 10, 2024

ibrahim-shehzad May 13, 2024 •

edited

Loading

caleb-johnson May 13, 2024

garrison left a comment

caleb-johnson commented May 10, 2024

caleb-johnson May 10, 2024

ibrahim-shehzad May 11, 2024

caleb-johnson May 11, 2024 •

edited

Loading

ibrahim-shehzad May 12, 2024 •

edited

Loading

caleb-johnson May 12, 2024 •

edited

Loading

ibrahim-shehzad May 13, 2024

caleb-johnson left a comment •

edited

Loading

garrison May 13, 2024

ibrahim-shehzad May 13, 2024

garrison May 14, 2024

ibrahim-shehzad May 14, 2024

garrison May 14, 2024

ibrahim-shehzad May 14, 2024 •

edited

Loading

garrison May 14, 2024

garrison May 14, 2024

ibrahim-shehzad May 14, 2024

garrison May 14, 2024

ibrahim-shehzad May 14, 2024

garrison May 14, 2024 •

edited

Loading

garrison left a comment

	def get_stats(self, penultimate=False) -> dict[str, NamedTuple \| None]:
	def get_stats(self, penultimate=False) -> dict[str, Any]:

Avoid exploring extraneous minima in the cut-finder search space #585

Avoid exploring extraneous minima in the cut-finder search space #585

Conversation

ibrahim-shehzad commented May 10, 2024 • edited Loading

coveralls commented May 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9083375618

Details

💛 - Coveralls

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

ibrahim-shehzad May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrison left a comment

Choose a reason for hiding this comment

caleb-johnson commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caleb-johnson May 11, 2024 • edited Loading

Choose a reason for hiding this comment

ibrahim-shehzad May 12, 2024 • edited Loading

Choose a reason for hiding this comment

caleb-johnson May 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caleb-johnson left a comment • edited Loading

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

ibrahim-shehzad May 14, 2024 • edited Loading

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

garrison May 14, 2024 • edited Loading

Choose a reason for hiding this comment

garrison left a comment

Choose a reason for hiding this comment

ibrahim-shehzad commented May 10, 2024 •

edited

Loading

coveralls commented May 10, 2024 •

edited

Loading

ibrahim-shehzad May 13, 2024 •

edited

Loading

caleb-johnson May 11, 2024 •

edited

Loading

ibrahim-shehzad May 12, 2024 •

edited

Loading

caleb-johnson May 12, 2024 •

edited

Loading

caleb-johnson left a comment •

edited

Loading

ibrahim-shehzad May 14, 2024 •

edited

Loading

garrison May 14, 2024 •

edited

Loading