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.

Sign up for GitHub

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

Jump to bottom

Suppress Opflow deprecation warnings in test_qpy.py #9998

Merged
merged 7 commits into from
Jul 17, 2023
Merged

Suppress Opflow deprecation warnings in test_qpy.py #9998

merged 7 commits into from
Jul 17, 2023

Conversation

t-imamichi
Copy link
Member

@t-imamichi t-imamichi commented Apr 20, 2023 •
edited
Loading

@t-imamichi t-imamichi requested a review from a team as a code owner April 20, 2023 09:15
@qiskit-bot
Copy link
Collaborator

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:

@coveralls
Copy link

coveralls commented Apr 20, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5152883879

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.001%) to 85.905%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 1 90.63%
qiskit/circuit/tools/pi_check.py 1 91.23%
qiskit/pulse/library/waveform.py 3 93.75%
Totals Coverage Status
Change from base Build 5148601299: 0.001%
Covered Lines: 71213
Relevant Lines: 82897
💛 - Coveralls

@mtreinish mtreinish added type: qa Issues and PRs that relate to testing and code quality Changelog: None Do not include in changelog labels Apr 20, 2023
jakelishman
jakelishman reviewed Apr 27, 2023
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

These deprecation warnings are fine, though would it be a good idea for us to start including the tests of what will be the new form of the functionality as well? I don't know if we have coverage of that, yet.

ElePT
ElePT reviewed May 4, 2023
View reviewed changes
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Sorry @t-imamichi that you have to clean up again after the opflow deprecation PR. As I explain in the specific comments, I think that we could already get rid of the opflow dependency in this test in particular.

@t-imamichi @ElePT
Apply suggestions from code review
4a0546b 
Co-authored-by: Elena Peña Tapia <[email protected]>
@t-imamichi
Copy link
Member Author

Thank you for your suggestions. I applied them.

ElePT
ElePT previously approved these changes May 15, 2023
View reviewed changes
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM!

@ElePT ElePT enabled auto-merge May 15, 2023 07:31
jakelishman
jakelishman reviewed May 24, 2023
View reviewed changes
test/qpy_compat/test_qpy.py Outdated
Comment on lines 130 to 136
def generate_string_parameters():
"""Generate a circuit from pauli tensor opflow."""
op_circuit = (X ^ Y ^ Z).to_circuit_op().to_circuit()
op_circuit.name = "X^Y^Z"
"""Generate a circuit for the XYZ pauli string."""
op_circuit = QuantumCircuit(3, name="X^Y^Z")
op_circuit.z(0)
op_circuit.y(1)
op_circuit.x(2)
return op_circuit
Copy link
Member

Choose a reason for hiding this comment

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

I think this change is actually breaking the intended test; in the original, there's a PauliGate in the circuit, which has a .params field that contains a string. The new form doesn't have that; it's just got a string in the op_circuit name, which is the standard.

We could probably hew to the intent of the test with something like

from qiskit.circuit.library import PauliGate

op_circuit = QuantumCircuit(3)
op_circuit.append(PauliGate("XYZ"), op_circuit.qubits, [])

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I use PauliGate 97f5a84.

@ElePT ElePT added this pull request to the merge queue Jul 17, 2023
jakelishman
jakelishman approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, this should make the tests a little less noisy now.

Merged via the queue into Qiskit:main with commit daa0bb5 Jul 17, 2023
@t-imamichi t-imamichi deleted the fix-opflow-warning branch July 18, 2023 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakelishman jakelishman jakelishman approved these changes

@ElePT ElePT ElePT left review comments

Assignees
No one assigned
Labels
Changelog: None Do not include in changelog type: qa Issues and PRs that relate to testing and code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@t-imamichi @qiskit-bot @coveralls @jakelishman @ElePT @mtreinish