-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
base: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 12424985899Details
💛 - Coveralls |
Hi @trigpolynom - thanks for fixing this bug! |
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 |
There was a problem hiding this comment.
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? 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
Hi @ShellyGarion and @Cryoris thanks for taking a look at this! I'll make the updates and requested changes tonight EST. |
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:
|
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) |
There was a problem hiding this comment.
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
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]) |
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`. |
There was a problem hiding this comment.
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.
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>`__. |
SparsePauliOp
initialization with dense Y
labels
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)