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

add dtype parameter to PauliSumOp.from_list #8896

Merged
merged 3 commits into from
Jan 10, 2023

Conversation

kevinsung
Copy link
Contributor

Summary

This change is needed for PauliSumOp to be able to pass type information to SparsePauliOp.

Details and comments

@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:

@woodsp-ibm woodsp-ibm added the mod: opflow Related to the Opflow module label Oct 13, 2022
@coveralls
Copy link

coveralls commented Oct 13, 2022

Pull Request Test Coverage Report for Build 3884574792

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 84.596%

Totals Coverage Status
Change from base Build 3882540095: -0.04%
Covered Lines: 64140
Relevant Lines: 75819

💛 - Coveralls

@ikkoham ikkoham added this to the 0.23.0 milestone Oct 21, 2022
@ikkoham ikkoham added the Changelog: New Feature Include in the "Added" section of the changelog label Oct 21, 2022
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.

Sounds good. Could you add the release note please?

BTWm, it might be nice to support Parameters in another PR:
https://github.com/Qiskit/qiskit-terra/blob/6aad91418951484ee5cf808a0c147f96bc951223/qiskit/opflow/primitive_ops/primitive_op.py#L229-L252

@Cryoris
Copy link
Contributor

Cryoris commented Oct 21, 2022

While this is certainly better than having to wrap the coefficients into an array, I still think it's not very intuitive for users if they have to explicitly set the dtype when using the parameters.

Would checking the type of each coefficient add a noticeable overhead? Isn't the complexity of initializing already linear? 🙂 Otherwise how about allowing to set the dtype for efficiency but checking the type for the user by default?

@kevinsung
Copy link
Contributor Author

Would checking the type of each coefficient add a noticeable overhead? Isn't the complexity of initializing already linear?

The overhead is pretty minimal. But I think it should be done in SparsePauliOp, not PauliSumOp. If that is agreed upon, I can make a PR for that.

@kevinsung kevinsung changed the title add dtype parameter to PauliSumOp.to_list add dtype parameter to PauliSumOp.from_list Oct 24, 2022
@kevinsung
Copy link
Contributor Author

I added the release note.

@Cryoris
Copy link
Contributor

Cryoris commented Oct 26, 2022

The overhead is pretty minimal. But I think it should be done in SparsePauliOp, not PauliSumOp. If that is agreed upon, I can make a PR for that.

@ikkoham what do you think about this, could we check the type of the coeffs on construction? 🙂

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Let's merge this since specifying the dtype is required to have the same functionality as the SparsePauliOp. We can follow-up if we should make the dtype be derived automatically so we can avoid explicitly setting it to object for parameters.

@mergify mergify bot merged commit 67a8937 into Qiskit:main Jan 10, 2023
@kevinsung kevinsung deleted the pauli-sum-op-dtype branch January 10, 2023 17:31
king-p3nguin pushed a commit to king-p3nguin/qiskit-terra that referenced this pull request Jan 11, 2023
* add dtype parameter to PauliSumOp.to_list

* add release note

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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 mod: opflow Related to the Opflow module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants