Skip to content

Update characterization and calibration experiments to use physical_qubits #1037

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

Merged
merged 12 commits into from
Feb 10, 2023

Conversation

wshanks
Copy link
Collaborator

@wshanks wshanks commented Feb 8, 2023

Summary

This PR completes the work started in #1031 to address #1000 by replacing all usage of qubits or qubit in the __init__ of BaseExperiment subclasses with physical_qubits.

Details and comments

All remaining experiment classes were updated following #1031's example, using deprecation warnings for the old argument form. All tests were also updated. As a sanity check, I ran the old version of the tests with deprecation warnings allowed and confirmed that the tests still passed. Usage in docstrings was also updated (harder to test for anything was missed) but usage in other docs was left for #983.

I also made some wording changes to the deprecation messages and updated BaseExperiment itself.

@coruscating Just checking -- the removal of support for the qubit argument is in 0.6? The other deprecated code in the repo is for after 0.6 (the plotting changes). So right after 0.5 releases soon, we can pull all the deprecations out of main?

@wshanks wshanks marked this pull request as ready for review February 8, 2023 21:12
@wshanks wshanks requested a review from coruscating February 8, 2023 21:12
Copy link
Collaborator

@coruscating coruscating left a comment

Choose a reason for hiding this comment

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

Thank you, this looks great! I'll make a separate PR to add some simple tests for the warnings. One thing I'm wondering about is whether calibration methods with qubits as an input, such as update_inst_map(), should be updated as well. What do you think?

@wshanks
Copy link
Collaborator Author

wshanks commented Feb 9, 2023

I missed the discussion on the qubits vs. physical_qubits decision, so I might not know all the reasoning behind it. I understand that physical_qubits is consisent with some things in terra like Target.phyiscal_qubits and CouplingMap.physical_qubits and helps to distinguish from the qubit indices in the template circuits that the experiment generates. With calibrations we do support using template circuits that get indices remapped, so the same reasoning would apply I think. On the other hand, maybe it is a little more clear with calibrations that you are calibrating physical qubits? I would be interested to know what @eggerdj, @nkanazawa1989, or others think.

What are you thinking for the tests? I wasn't sure what to add. We could add something that passes qubit to T1 and confirm that it works with a warning. I had been thinking that we were about to release and would immediately pull that test back out since the deprecation says support will be removed after the next release. I guess it could keep living in the release branch though in case there are point releases.

@wshanks wshanks merged commit 929094a into qiskit-community:main Feb 10, 2023
@wshanks wshanks deleted the physical_qubits branch February 10, 2023 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants