Make heuristic score sorting robust in 1q optimization pass #9239
Conversation
In Qiskit#8197 the Optimize1qGatesDecomposition was updated to be target aware and as part of that a new heuristic scoring was added to determine which decomposition was the best performing and should be used. However, in the case of an error rate of 0.0 the desired behavior was to pick the decomposition which was shortest. In Qiskit#8197 this was accomplished by just doing `-100 + len(decomposition)` which worked in the general case as long as the gate counts are < 100 (which in practice will always be the case). For robustness this changes the return for the scoring function to be `(error_rate, sequence_length)` to give us the sorting without changing the measured error rate.
|
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 3605288117
💛 - Coveralls |
| new_error = _error(new_circ, self._target, qubit) | ||
| if isinstance(new_error, tuple): | ||
| error_rate = new_error[0] | ||
| else: | ||
| error_rate = new_error |
There was a problem hiding this comment.
If target is None, the output of _error is the length of the circuit, which isn't really an error rate. Is that what you meant to happen? If so, we could change the function to just return (len(circuit),) instead to simplify some checking.
| or np.isclose(_error(new_circ, self._target, qubit), 0) | ||
| or np.isclose(error_rate, 0) |
There was a problem hiding this comment.
This check feels weird, because before this PR the last line could only have triggered if the error rate was "close to" 0, but not actually 0 - in that case, it would have been replaced by -100 + len(circuit), so not be close any more. I'm not sure if that was intended - the behaviour it'll have after this PR (anything "close to" zero triggers the replacement) feels more natural to me.
| @@ -112,13 +112,18 @@ def _substitution_checks(self, dag, old_run, new_circ, basis, qubit): | |||
| # if we're outside of the basis set, we're obligated to logically decompose. | |||
| # if we're outside of the set of gates for which we have physical definitions, | |||
| # then we _try_ to decompose, using the results if we see improvement. | |||
| new_error = _error(new_circ, self._target, qubit) | |||
There was a problem hiding this comment.
Not sure if it's overkill, but I think it might be cleaner and less error-prone to add a local class to encapsulate the error tuple, and then generate a total ordering for it with @functools.total_ordering.
It looks like I'd worry there might be times where code accidentally compares tuples to numbers._error is used by _resynthesize_run as a key to min so
EDIT: crossed-out isn't relevant (didn't notice at first glance that the target is bound as the same for all).
There was a problem hiding this comment.
Tuple[T, ...] has an automatic total ordering in Python if type T has a total ordering itself, regardless of whether the lengths match - using tuples in comparisons is fairly idiomatic (e.g. if sys.version_info < (3, 10) etc). Maybe I'm misunderstanding what you meant, though.
|
I baked this into a larger refactor of this pass in #9578 so we can close this PR as it's not needed anymore. |
Summary
In #8197 the Optimize1qGatesDecomposition was updated to be target aware and as part of that a new heuristic scoring was added to determine which decomposition was the best performing and should be used. However, in the case of an error rate of 0.0 the desired behavior was to pick the decomposition which was shortest. In #8197 this was accomplished by just doing
-100 + len(decomposition)which worked in the general case as long as the gate counts are < 100 (which in practice will always be the case). For robustness this changes the return for the scoring function to be(error_rate, sequence_length)to give us the sorting without changing the measured error rate.Details and comments