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

Allow multiplication of SparsePauliOp and ParameterExpression #10264

Merged
merged 7 commits into from
Jun 26, 2023
Merged

Allow multiplication of SparsePauliOp and ParameterExpression #10264

merged 7 commits into from
Jun 26, 2023

Conversation

SimoneGasperini
Copy link
Contributor

Summary

Fix issue #10185 relying on the already implemented and tested method SparsePauliOp._multiply.

@SimoneGasperini SimoneGasperini requested review from a team and ikkoham as code owners June 12, 2023 15:20
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 12, 2023
@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:

  • @Qiskit/terra-core
  • @ikkoham

@woodsp-ibm woodsp-ibm added the mod: quantum info Related to the Quantum Info module (States & Operators) label Jun 12, 2023
@woodsp-ibm
Copy link
Member

This should have unit test(s) to verify that multiplication by a parameter expression works, and continues to do so, as expected. This should also have a release note informing about this new feature.

@SimoneGasperini
Copy link
Contributor Author

Thank you @woodsp-ibm for the suggestions. Should I write a separate testing function for this or add it to the current TestSparsePauliOpMethods.test_mul method in test_sparse_pauli_op.py?

@woodsp-ibm
Copy link
Member

If adding new parameterexpression value(s) to the current test works out with the current code then that would be ok. It already deals with a param and having that now as value I do not know how well things would fit to the code given how you might want/need to check the outcome. If it turns out better to be a separate test, save complicating what's there, I think that's fine.

Copy link
Contributor

@ikkoham ikkoham 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 for the PR. Agree with @woodsp-ibm, it needs some tests. The dtype of the coeffs must be handled carefully.

Note added: also needs a release note for the new feature.

@ikkoham ikkoham added the Changelog: New Feature Include in the "Added" section of the changelog label Jun 13, 2023
@SimoneGasperini
Copy link
Contributor Author

Thank you again for the feedback. I added the tests and the release note.
Some tests are still failing but they look related to the other changes from the 'main' branch. Maybe I shouldn't have merged it to my feature branch (apologies, I'm just at the beginning of my experience as an open-source contributor).

@woodsp-ibm
Copy link
Member

Hi, as far as the test failure sometimes other changes affect things - in this case its failing due to a change in newly released qiskit aer - #10270 addresses this so after thats merged and your branch is updated to include it then things should be ok.

Copy link
Contributor

@ikkoham ikkoham 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 for adding tests and a release note. LGTM!

@ikkoham ikkoham added this pull request to the merge queue Jun 26, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 5257229470

  • 1 of 2 (50.0%) changed or added relevant lines in 1 file are covered.
  • 12 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.007%) to 85.9%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py 1 2 50.0%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 1 90.39%
qiskit/extensions/quantum_initializer/squ.py 2 80.0%
crates/qasm2/src/lex.rs 3 90.38%
crates/qasm2/src/parse.rs 6 95.71%
Totals Coverage Status
Change from base Build 5256241220: -0.007%
Covered Lines: 71289
Relevant Lines: 82991

💛 - Coveralls

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 26, 2023
@jakelishman jakelishman added this pull request to the merge queue Jun 26, 2023
Merged via the queue into Qiskit:main with commit c693852 Jun 26, 2023
@SimoneGasperini SimoneGasperini deleted the sparsepauliop_mul_parameter branch June 27, 2023 07:59
jlapeyre pushed a commit to ewinston/qiskit that referenced this pull request Jun 28, 2023
…#10264)

* Allow multiplication of SparsePauliOp and ParameterExpression

* Add unit tests for SparsePauliOp and ParameterExpression multiplication

* Add release note for the new feature
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: quantum info Related to the Quantum Info module (States & Operators)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants