-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Undo breaking API change in unitary synthesis plugin interface #10066
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.
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
Conversation
This commit reverts an unecessary breaking API change that was introduced in Qiskit#9175. In that PR the data type for two arguments, gate_lengths and gate_errors, that was potentially passed to plugins was changed. This change was well intentioned as it was used by the default plugin as part of the changes to the default plugin made in that PR. But this neglected the downstream effects for any existing plugin interface users relying on the input to those fields. Making this change to existing fields would break any plugins that were using the old data format and is a violation of the Qiskit stability and deprecation policy. This commit reverts that change so that gate_lengths and gate_errors contain the same data format as in previous releases. Instead to facilitate the new workflow a new fields, gate_lengths_by_qubit and gate_errors_by_qubit, were added that leverage the new format introduced by Qiskit#9175. By adding this as new optional fields existing users can continue to work as before, but the default plugin which requires the different data format can use the new data type.
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 4875941352
💛 - Coveralls |
I did add the API change label and upgrade releasenote in that PR. |
Yeah, you did the right things to document a breaking change, but that doesn't mean we really should be breaking the API in this case. I don't really know if there are any plugins out there using these interfaces either, but to a certain degree we can't really know for sure because there is no requirement for any downstream plugin interface users to publish or document their usage. If there isn't any value in having the old view moving forward we can deprecate it in 0.25.0 (we have to release the alternative first before marking it as deprecated). The key though is that we give any plugin authors/maintainers a chance to migrate before removing the API they've potentially been relying on. Although, in this case I'm personally unclear if there is any harm in keeping both views around long term there really isn't a guarantee that a plugin supports synthesizing to heterogeneous basis gates (lol or that it even respects the basis gates set in the target), but we can discuss that in a deprecation PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two documentation comments, otherwise LGTM.
* Undo breaking API change in unitary synthesis plugin interface This commit reverts an unecessary breaking API change that was introduced in #9175. In that PR the data type for two arguments, gate_lengths and gate_errors, that was potentially passed to plugins was changed. This change was well intentioned as it was used by the default plugin as part of the changes to the default plugin made in that PR. But this neglected the downstream effects for any existing plugin interface users relying on the input to those fields. Making this change to existing fields would break any plugins that were using the old data format and is a violation of the Qiskit stability and deprecation policy. This commit reverts that change so that gate_lengths and gate_errors contain the same data format as in previous releases. Instead to facilitate the new workflow a new fields, gate_lengths_by_qubit and gate_errors_by_qubit, were added that leverage the new format introduced by #9175. By adding this as new optional fields existing users can continue to work as before, but the default plugin which requires the different data format can use the new data type. * Update docstring * Fix copy paste error (cherry picked from commit 72e7481)
… (#10075) * Undo breaking API change in unitary synthesis plugin interface This commit reverts an unecessary breaking API change that was introduced in #9175. In that PR the data type for two arguments, gate_lengths and gate_errors, that was potentially passed to plugins was changed. This change was well intentioned as it was used by the default plugin as part of the changes to the default plugin made in that PR. But this neglected the downstream effects for any existing plugin interface users relying on the input to those fields. Making this change to existing fields would break any plugins that were using the old data format and is a violation of the Qiskit stability and deprecation policy. This commit reverts that change so that gate_lengths and gate_errors contain the same data format as in previous releases. Instead to facilitate the new workflow a new fields, gate_lengths_by_qubit and gate_errors_by_qubit, were added that leverage the new format introduced by #9175. By adding this as new optional fields existing users can continue to work as before, but the default plugin which requires the different data format can use the new data type. * Update docstring * Fix copy paste error (cherry picked from commit 72e7481) Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
…t#10066) * Undo breaking API change in unitary synthesis plugin interface This commit reverts an unecessary breaking API change that was introduced in Qiskit#9175. In that PR the data type for two arguments, gate_lengths and gate_errors, that was potentially passed to plugins was changed. This change was well intentioned as it was used by the default plugin as part of the changes to the default plugin made in that PR. But this neglected the downstream effects for any existing plugin interface users relying on the input to those fields. Making this change to existing fields would break any plugins that were using the old data format and is a violation of the Qiskit stability and deprecation policy. This commit reverts that change so that gate_lengths and gate_errors contain the same data format as in previous releases. Instead to facilitate the new workflow a new fields, gate_lengths_by_qubit and gate_errors_by_qubit, were added that leverage the new format introduced by Qiskit#9175. By adding this as new optional fields existing users can continue to work as before, but the default plugin which requires the different data format can use the new data type. * Update docstring * Fix copy paste error
Summary
This commit reverts an unecessary breaking API change that was introduced in #9175. In that PR the data type for two arguments, gate_lengths and gate_errors, that was potentially passed to plugins was changed. This change was well intentioned as it was used by the default plugin as part of the changes to the default plugin made in that PR. But this neglected the downstream effects for any existing plugin interface users relying on the input to those fields. Making this change to existing fields would break any plugins that were using the old data format and is a violation of the Qiskit stability and deprecation policy. This commit reverts that change so that gate_lengths and gate_errors contain the same data format as in previous releases. Instead to facilitate the new workflow a new fields, gate_lengths_by_qubit and gate_errors_by_qubit, were added that leverage the new format introduced by #9175. By adding this as new optional fields existing users can continue to work as before, but the default plugin which requires the different data format can use the new data type.
Details and comments