Add MinimumPoint transpiler pass #9612
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mtreinish
added
Changelog: New Feature
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 4565674227
💛 - Coveralls |
1ucian0
reviewed
Mar 3, 2023
1ucian0
reviewed
Mar 4, 2023
1ucian0
reviewed
Mar 4, 2023
1ucian0
reviewed
Mar 11, 2023
Comment on lines
42
to
51
| * ``{prefix}_minimum_point_count`` - Used to track the number of executions since | ||
| the current minimum was found | ||
| * ``{prefix}_backtrack_history`` - Stores the current minimum value and :class:`~.DAGCircuit` | ||
| * ``{prefix}_minimum_point`` - This value gets set to ``True`` when either a fixed point | ||
| is reached over the ``backtrack_depth`` executions, or ``backtrack_depth`` was exceeded | ||
| and an earlier minimum is restored. | ||
| """ |
There was a problem hiding this comment.
This bullet list seems to not render correctly:
There was a problem hiding this comment.
there's an extra space in the indentation, which is causing it to be parsed as a bullet point containing a single-item definition list
1ucian0
reviewed
Mar 11, 2023
jakelishman
reviewed
Mar 13, 2023
There was a problem hiding this comment.
I'm happy with the logic of what this PR is trying to accomplish, and I'm happy that it'll be superior to what we've got in place already, although maybe we should replace all use of FixedPoint with this - it feels like an accident waiting to happen in other places too. My issues are mostly just with legibility - the logic is quite tricksy, so I think it's important that we put a good amount of effort into making both it and its tests as readable as possible.
Comment on lines
108
to
179
| def test_min_reset_backtrack_range(self): | ||
| """Test minimum resets backtrack depth.""" | ||
| min_pass = MinimumPoint(["fidelity", "depth", "size"], prefix="test") | ||
| dag = DAGCircuit() | ||
| min_pass.property_set["fidelity"] = 0.875 | ||
| min_pass.property_set["depth"] = 15 | ||
| min_pass.property_set["size"] = 20 | ||
| min_pass.run(dag) | ||
| self.assertEqual(min_pass.property_set["test_minimum_point_count"], 1) | ||
| self.assertIsNone(min_pass.property_set["test_backtrack_history"]) | ||
| self.assertIsNone(min_pass.property_set["test_min_point"]) |
There was a problem hiding this comment.
Some of the assertions in these tests feel rather like testing relatively minor implementation details, like the exact objects we expect are tracked with very specific names in the property set.
Leaving that aside, though, the test is quite hard to quickly check what the sequence of values are being set as the properties; for something like this, I think it's maybe easier if we could define the sequence of inserted values and expected values up in single lists at the top of the test so all the context is localised, then loop through them.
There was a problem hiding this comment.
I left the tests mostly in place like this primarily to make it explicit what is happening in each iteration. I worry the details of the exact logic flow would be lost with a for loop. Especially given the complexity of the logic going on here my thinking was it might be difficult to track and testing it explicitly like this made it easier for me to ensure I got the expected logic. I agree that it does end up being a bit repetitive and a bit too detailed. In e2c8cd0 I added a bunch of comments to each iteration to explain the context.
This commit adds a new transpiler pass MinimumPoint which is used to find a local minimum point from the property set between executions of the pass. This is similar to the existing FixedPoint pass but will find the minimum fixed point over the past n exeuctions as opposed to finding a when two subsequent exectuions are at the same point. This is then used in optimization level 3 because the 2q unitary synthesis optimization that is part of the optimization loop can occasionally get stuck oscillating between multiple different equivalent decompositions which prevents the fixed point condition from ever being reached. By checking that we've reached the minimum over the last 5 executions we ensure we're reaching an exit condition in this situation. Fixes Qiskit#5832 Fixes Qiskit#9177 (the underlying cause of the optimization loop's inability to converge at optimization level 3 is not fixed here, there is still a root cause of instability in UnitarySynthesis this just changes the loop exit condition so we're never stuck in an infinte loop)
This commit reworks the logic of the pass to track the state via a dataclass instead of using separate property set fields. This cleans up the code for dealing with checking the current state relative to earlier iterations by making the access just attributes instead of secret strings. At the same time the tests were updated to handle this new data structure comments were added to better explain the logic flow being tested.
mtreinish
force-pushed
the
minimum-point-pass
branch
from
44b6b4e
to
e2c8cd0
Compare
jakelishman
reviewed
Mar 29, 2023
ElePT
pushed a commit
to ElePT/qiskit
that referenced
this pull request
Apr 5, 2023
* Add MinimumPoint transpiler pass This commit adds a new transpiler pass MinimumPoint which is used to find a local minimum point from the property set between executions of the pass. This is similar to the existing FixedPoint pass but will find the minimum fixed point over the past n exeuctions as opposed to finding a when two subsequent exectuions are at the same point. This is then used in optimization level 3 because the 2q unitary synthesis optimization that is part of the optimization loop can occasionally get stuck oscillating between multiple different equivalent decompositions which prevents the fixed point condition from ever being reached. By checking that we've reached the minimum over the last 5 executions we ensure we're reaching an exit condition in this situation. Fixes Qiskit#5832 Fixes Qiskit#9177 (the underlying cause of the optimization loop's inability to converge at optimization level 3 is not fixed here, there is still a root cause of instability in UnitarySynthesis this just changes the loop exit condition so we're never stuck in an infinte loop) * Doc copy-paste error cleanups * Rework logic to track state with a dataclass This commit reworks the logic of the pass to track the state via a dataclass instead of using separate property set fields. This cleans up the code for dealing with checking the current state relative to earlier iterations by making the access just attributes instead of secret strings. At the same time the tests were updated to handle this new data structure comments were added to better explain the logic flow being tested. * Fix doc typo * Apply suggestions from code review Co-authored-by: Jake Lishman <[email protected]> * Update slots syntax for dataclass to be compatible with Python < 3.10 * Update property set check in tests --------- Co-authored-by: Jake Lishman <[email protected]>
giacomoRanieri
pushed a commit
to giacomoRanieri/qiskit-terra
that referenced
this pull request
Apr 16, 2023
* Add MinimumPoint transpiler pass This commit adds a new transpiler pass MinimumPoint which is used to find a local minimum point from the property set between executions of the pass. This is similar to the existing FixedPoint pass but will find the minimum fixed point over the past n exeuctions as opposed to finding a when two subsequent exectuions are at the same point. This is then used in optimization level 3 because the 2q unitary synthesis optimization that is part of the optimization loop can occasionally get stuck oscillating between multiple different equivalent decompositions which prevents the fixed point condition from ever being reached. By checking that we've reached the minimum over the last 5 executions we ensure we're reaching an exit condition in this situation. Fixes Qiskit#5832 Fixes Qiskit#9177 (the underlying cause of the optimization loop's inability to converge at optimization level 3 is not fixed here, there is still a root cause of instability in UnitarySynthesis this just changes the loop exit condition so we're never stuck in an infinte loop) * Doc copy-paste error cleanups * Rework logic to track state with a dataclass This commit reworks the logic of the pass to track the state via a dataclass instead of using separate property set fields. This cleans up the code for dealing with checking the current state relative to earlier iterations by making the access just attributes instead of secret strings. At the same time the tests were updated to handle this new data structure comments were added to better explain the logic flow being tested. * Fix doc typo * Apply suggestions from code review Co-authored-by: Jake Lishman <[email protected]> * Update slots syntax for dataclass to be compatible with Python < 3.10 * Update property set check in tests --------- Co-authored-by: Jake Lishman <[email protected]>
king-p3nguin
pushed a commit
to king-p3nguin/qiskit-terra
that referenced
this pull request
May 22, 2023
* Add MinimumPoint transpiler pass This commit adds a new transpiler pass MinimumPoint which is used to find a local minimum point from the property set between executions of the pass. This is similar to the existing FixedPoint pass but will find the minimum fixed point over the past n exeuctions as opposed to finding a when two subsequent exectuions are at the same point. This is then used in optimization level 3 because the 2q unitary synthesis optimization that is part of the optimization loop can occasionally get stuck oscillating between multiple different equivalent decompositions which prevents the fixed point condition from ever being reached. By checking that we've reached the minimum over the last 5 executions we ensure we're reaching an exit condition in this situation. Fixes Qiskit#5832 Fixes Qiskit#9177 (the underlying cause of the optimization loop's inability to converge at optimization level 3 is not fixed here, there is still a root cause of instability in UnitarySynthesis this just changes the loop exit condition so we're never stuck in an infinte loop) * Doc copy-paste error cleanups * Rework logic to track state with a dataclass This commit reworks the logic of the pass to track the state via a dataclass instead of using separate property set fields. This cleans up the code for dealing with checking the current state relative to earlier iterations by making the access just attributes instead of secret strings. At the same time the tests were updated to handle this new data structure comments were added to better explain the logic flow being tested. * Fix doc typo * Apply suggestions from code review Co-authored-by: Jake Lishman <[email protected]> * Update slots syntax for dataclass to be compatible with Python < 3.10 * Update property set check in tests --------- Co-authored-by: Jake Lishman <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This commit adds a new transpiler pass MinimumPoint which is used to find a local minimum point from the property set between executions of the pass. This is similar to the existing FixedPoint pass but will find the minimum fixed point over the past n exeuctions as opposed to finding a when two subsequent exectuions are at the same point. This is then used in optimization level 3 because the 2q unitary synthesis optimization that is part of the optimization loop can occasionally get stuck oscillating between multiple different equivalent decompositions which prevents the fixed point condition from ever being reached. By checking that we've reached the minimum over the last 5 executions we ensure we're reaching an exit condition in this situation.
Details and comments
Fixes #5832
Fixes #9177
Fixes #9776
(the underlying cause of the optimization loop's inability to converge at optimization level 3 is not fixed here, there is still a root cause of instability in
UnitarySynthesisthis just changes the loop exit condition so we're never stuck in an infinte loop)