Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
May 14, 2024
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5911b10
Avoid exploring extraneous minima in the search space
ibrahim-shehzad May 10, 2024
bbd5d11
fix failing test
ibrahim-shehzad May 10, 2024
59eab4f
fix coverage
ibrahim-shehzad May 10, 2024
16d2a13
black
ibrahim-shehzad May 10, 2024
580b4a9
update doc string
ibrahim-shehzad May 10, 2024
5998d8f
update doc string
ibrahim-shehzad May 10, 2024
c4c2968
add new tests and modify states check
ibrahim-shehzad May 13, 2024
f2a8ad6
Merge branch 'avoid-exploring-multiple-minima' of https://github.com/…
ibrahim-shehzad May 13, 2024
603af4a
update test description
ibrahim-shehzad May 13, 2024
16ecc0f
style
ibrahim-shehzad May 13, 2024
837875c
change to namedtuple, add release note
ibrahim-shehzad May 14, 2024
6ce2d5e
update return
ibrahim-shehzad May 14, 2024
d931ec6
change type hints
ibrahim-shehzad May 14, 2024
d774c5b
Merge branch 'avoid-exploring-multiple-minima' of https://github.com/…
ibrahim-shehzad May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions circuit_knitting/cutting/cut_finding/best_first_search.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 non-LO QPD assignments, it always makes sense to stop once
the first minimum has been reached and therefore, we set this bool to ``True``.

``max_backjumps`` (int or None) is the maximum number of backjump operations that
can be performed before the search is forced to terminate. None indicates
Expand Down Expand Up @@ -185,7 +187,7 @@ def __init__(
self,
optimization_settings: OptimizationSettings,
search_functions: SearchFunctions,
stop_at_first_min: bool = False,
stop_at_first_min: bool = True,
):
"""Initialize an instance of :class:`BestFirstSearch`.

Expand Down Expand Up @@ -258,7 +260,6 @@ def optimization_pass(
self.mincost_bound = self.mincost_bound_func(*args) # type: ignore

prev_depth = None

while (
self.pqueue.qsize() > 0
and (not self.stop_at_first_min or not self.min_reached)
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@ibrahim-shehzad ibrahim-shehzad May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good test, thanks

return None, None

self.num_states_visited += 1
Expand Down
2 changes: 1 addition & 1 deletion circuit_knitting/cutting/cut_finding/cut_optimization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ def __init__(
"CutOptimization",
self.settings,
self.search_funcs,
stop_at_first_min=False,
stop_at_first_min=True,
)
sq.initialize([start_state], self.func_args)

Expand Down
14 changes: 7 additions & 7 deletions docs/circuit_cutting/tutorials/04_automatic_cut_finding.ipynb
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion test/cutting/cut_finding/test_cut_finder_results.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@caleb-johnson caleb-johnson May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@ibrahim-shehzad ibrahim-shehzad May 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@caleb-johnson caleb-johnson May 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

).all() # matches known stats.


Expand Down