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

Fix SparsePauliOp initialization with dense Y labels #13580

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

trigpolynom
Copy link
Contributor

@trigpolynom trigpolynom commented Dec 18, 2024

Summary

Fixes #13522

"When a SparsePauliOp object is instantiated with a long string of Y's (greater than 100), the imaginary component of the coefficients is non-zero (but trivially small, ~1e-15). This results in errors when submitting jobs using qiskit-ibm-runtime which throws an error when using the SparsePauliOp as the expectation value to estimate."

The fix to the above problem involved using modular arithmetic to calculate coeffs for the SparsePauliOp when coeffs is None. This avoids the floating point error that occurs for instantiating heavy-weight SparsePauliOps.

Details and comments

Added a test method test_sparse_pauli_op_init_long_ys to ensure that the error causing PauliList ('Y"*100) results with the same coeffs as PauliList ("Y"*99)

@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Dec 18, 2024
@coveralls
Copy link

coveralls commented Dec 18, 2024

Pull Request Test Coverage Report for Build 12424985899

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 11 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.004%) to 88.947%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 92.2%
crates/qasm2/src/lex.rs 4 91.98%
crates/qasm2/src/parse.rs 6 97.15%
Totals Coverage Status
Change from base Build 12420636821: -0.004%
Covered Lines: 79426
Relevant Lines: 89296

💛 - Coveralls

@ShellyGarion ShellyGarion added mod: quantum info Related to the Quantum Info module (States & Operators) Changelog: Bugfix Include in the "Fixed" section of the changelog labels Dec 19, 2024
@ShellyGarion
Copy link
Member

Hi @trigpolynom - thanks for fixing this bug!
After you complete the tests and fix the lint error, could you please add release notes?

self._coeffs = np.asarray((-1j) ** (phase - count_y) * coeffs, dtype=coeffs.dtype)

# Compute exponentiation via integer arithmetic and lookup table to avoid floating point errors
exp_val = (phase - count_y) % 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Also a tiny nitpick -- exp_val to me sounds like an expectation value, as we're in the realm of quantum computing. Would just exponent be a more precise name? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes makes complete sense. See update.

Copy link
Member

Choose a reason for hiding this comment

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

note that there is still a lint error (see here)

************* Module qiskit.quantum_info.operators.symplectic.sparse_pauli_op
qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py:171:0: C0301: Line too long (107/105) (line-too-long)

@trigpolynom
Copy link
Contributor Author

Hi @ShellyGarion and @Cryoris thanks for taking a look at this! I'll make the updates and requested changes tonight EST.

@trigpolynom trigpolynom marked this pull request as ready for review December 20, 2024 03:27
@trigpolynom trigpolynom requested a review from a team as a code owner December 20, 2024 03:27
@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 following people are relevant to this code:

  • @Qiskit/terra-core

Comment on lines +145 to +150
with self.subTest(msg="make SparsePauliOp from string of Y's"):
yyy = SparsePauliOp("Y" * 100)
print(yyy.coeffs)
yy = SparsePauliOp("Y" * 99)
print(yy.coeffs)
self.assertEqual(yyy.coeffs, yy.coeffs)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need a subTest since there's only a single assertion statement in the function, also I think it would be better to use a fixed comparison value -- theoretically both 99 and 100 Y terms could be equal but wrong 🙂 We could e.g. just do

Suggested change
with self.subTest(msg="make SparsePauliOp from string of Y's"):
yyy = SparsePauliOp("Y" * 100)
print(yyy.coeffs)
yy = SparsePauliOp("Y" * 99)
print(yy.coeffs)
self.assertEqual(yyy.coeffs, yy.coeffs)
y = SparsePauliOp("Y" * 1000)
self.assertEqual(1, y.coeffs[0])

Comment on lines +3 to +7
Fixed a bug where a floating point arithmetic error caused a heavy-weight
initialization of :class:`.SparsePauliOp` (pauli list of size > 99) without any
specified coeffs, to have non-zero imaginary components. The fix involved using
modular arithmetic to set the correct coefficients based on the phase and count_y of
the :class:`.PauliList`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding a reno! Note that we don't typically explain how the bug was fixed if it is a technical detail. However we do include the issue that has been fixed, so users can have a look at more detailed discussions, if there are any.

Suggested change
Fixed a bug where a floating point arithmetic error caused a heavy-weight
initialization of :class:`.SparsePauliOp` (pauli list of size > 99) without any
specified coeffs, to have non-zero imaginary components. The fix involved using
modular arithmetic to set the correct coefficients based on the phase and count_y of
the :class:`.PauliList`.
Fixed a bug where a initializing :class:`.SparsePauliOp` with a large
number of Pauli-``Y`` terms (typically :math:`\geq 100`) and no explicit
``coeffs`` would result in a coefficient close to 1 but with a floating point
error. The coefficient is now correctly 1 per default.
Fixed `#13522 <https://github.com/Qiskit/qiskit/issues/13522>`__.

@Cryoris Cryoris changed the title Fix sparse pauli op amps Fix SparsePauliOp initialization with dense Y labels Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" 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: No status
Development

Successfully merging this pull request may close these issues.

SparsePauliOp incorrectly sets non-zero imaginary component
5 participants