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.

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

Adopt qiskit.result.mitigation into qiskit_experiments.data_processing #1484

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

wshanks
Copy link
Collaborator

@wshanks wshanks commented Nov 8, 2024

The readout error mitigator classes LocalReadoutMitigator and
CorrelatedReadoutMitigator have been copied into
qiskit_experiments.data_processing.mitigation from
qiskit.result.mitigation in order to prepare for the deprecation of
these classes from Qiskit. The experiments that previously used the
Qiskit versions of these mitigator classes now use the Experiments
versions. The manual has also been updated to use the new class
locations.

Additionally, the readout mitigation manual was refactored to avoid using private functions and methods. Some adjacent jupyter-execute cells were merged because in the rendered docs they look like one cell any way, but they still have individual "copy to clipboard" buttons which is confusing. You think clicking the button will get the merged cell contents but it only gets a section of it.

@wshanks
Copy link
Collaborator Author

wshanks commented Nov 8, 2024

This is a draft for now because it includes the changes from #1483 as well and is intended as a follow up.

@wshanks wshanks added the backport stable potential The issue or PR might be minimal and/or import enough to backport to stable label Nov 8, 2024
@wshanks
Copy link
Collaborator Author

wshanks commented Nov 8, 2024

I also pushed this branch into the main repo so I could run the Cron-Staging workflow and it passed.

@@ -102,9 +93,9 @@ The individual mitigation matrices can be read off the mitigator.

.. jupyter-execute::

for m in mitigator._mitigation_mats:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Besides changing the import location of the mitigators, I made some small changes to this manual to stop using private functions and methods.

@@ -118,13 +109,9 @@ Mitigation example
qc.cx(i - 1, i)
qc.measure_all()

.. jupyter-execute::
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 also merged adjacent jupyter-execute cells because they don't work well. In the rendered docs, they look like one big cell any way, but they still have individual buttons for copying the code to the clipboard, so you think you are copying the whole merged cell but you only get a subsection of it.

Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

LGTM. I didn't check diff of the mitigation module from the original qiskit core implementation. I believe the logic is identical, but let me know if you add or modify functionalities.

I'll approve after draft status is removed.

@@ -143,16 +130,31 @@ Expectation value

.. jupyter-execute::

def str2diag(string):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we have to define this function although you port this function to .data_processing.mitigation.utils?

Instead of using the util function, we can also write:

from qiskit.quantum_info import Operator
Operator.from_label("II0I").to_matrix()

which looks much simpler.

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 didn't want to make .data_processing.mitigation.utils public and str2diag was the only code that I did not know how to replace with other public code. I thought there was probably something in Qiskit but I had not figured it out. I can try Operator.from_label.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nkanazawa1989 Operator.from_label worked. This is ready for review now.

@wshanks
Copy link
Collaborator Author

wshanks commented Nov 14, 2024

I didn't check diff of the mitigation module from the original qiskit core implementation. I believe the logic is identical, but let me know if you add or modify functionalities.

No, it should be a straight copy from Qiskit besides changing some imports.

The readout error mitigator classes `LocalReadoutMitigator` and
`CorrelatedReadoutMitigator` have been copied into
`qiskit_experiments.data_processing.mitigation` from
`qiskit.result.mitigation` in order to prepare for the deprecation of
these classes from Qiskit. The experiments that previously used the
Qiskit versions of these mitigator classes now use the Experiments
versions. The manual has also been updated to use the new class
locations.
@wshanks wshanks marked this pull request as ready for review November 14, 2024 19:18
Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@wshanks wshanks added this pull request to the merge queue Nov 15, 2024
Merged via the queue into qiskit-community:main with commit 940b94d Nov 15, 2024
11 checks passed
mergify bot pushed a commit that referenced this pull request Nov 15, 2024
…sing` (#1484)

The readout error mitigator classes `LocalReadoutMitigator` and
`CorrelatedReadoutMitigator` have been copied into
`qiskit_experiments.data_processing.mitigation` from
`qiskit.result.mitigation` in order to prepare for the deprecation of
these classes from Qiskit. The experiments that previously used the
Qiskit versions of these mitigator classes now use the Experiments
versions. The manual has also been updated to use the new class
locations.

Additionally, the readout mitigation manual was refactored to avoid
using private functions and methods. Some adjacent `jupyter-execute`
cells were merged because in the rendered docs they look like one cell
any way, but they still have individual "copy to clipboard" buttons
which is confusing. You think clicking the button will get the merged
cell contents but it only gets a section of it.

(cherry picked from commit 940b94d)
@wshanks wshanks deleted the adopt-mitigators branch November 15, 2024 00:38
wshanks added a commit that referenced this pull request Nov 15, 2024
…sing` (backport #1484) (#1486)

The readout error mitigator classes `LocalReadoutMitigator` and
`CorrelatedReadoutMitigator` have been copied into
`qiskit_experiments.data_processing.mitigation` from
`qiskit.result.mitigation` in order to prepare for the deprecation of
these classes from Qiskit. The experiments that previously used the
Qiskit versions of these mitigator classes now use the Experiments
versions. The manual has also been updated to use the new class
locations.

Additionally, the readout mitigation manual was refactored to avoid
using private functions and methods. Some adjacent `jupyter-execute`
cells were merged because in the rendered docs they look like one cell
any way, but they still have individual "copy to clipboard" buttons
which is confusing. You think clicking the button will get the merged
cell contents but it only gets a section of it.<hr>This is an automatic
backport of pull request #1484 done by [Mergify](https://mergify.com).

Co-authored-by: Will Shanks <willshanks@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport stable potential The issue or PR might be minimal and/or import enough to backport to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants