-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add SparseObservable support to evolved_operator_ansatz #15442
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
base: main
Are you sure you want to change the base?
Conversation
- Add conditional import and handling for SparseObservable - Extend Rust fast path to accept SparseObservable (in addition to SparsePauliOp) - Enhance _is_pauli_identity() to handle SparseObservable with coefficient checks - Fix edge cases: return empty circuit when all operators filtered, handle string/list parameter_prefix - Add comprehensive test coverage for SparseObservable support
|
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:
|
Cryoris
left a comment
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.
This PR is over-engineering the solution a lot, please check the comments below to simplify the code.
- Use direct import of SparseObservable (no try-except) - Simplify identity check: use num_terms == 1 and len(bit_labels()) == 0 - Fix empty circuit handling: return circuit with correct num_qubits when all operators are identities - Add SparseObservable support to fast Rust path - Fix prefix handling to support both string and list types - Add comprehensive test cases for all changes
|
Hi, I’ve updated and simplified the code according to your comments. |
Cryoris
left a comment
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 the updates @akhil-102, the approach looks good now! Also great catch on the bug with the string-based parameter prefix. It would be great to separate the fix into a different PR, if you'd be interested in doing that -- see also the comments below.
| def setUp(self): | ||
| """Set up test fixtures.""" | ||
| super().setUp() | ||
| try: | ||
| from qiskit.quantum_info import SparseObservable | ||
| self.SparseObservable = SparseObservable | ||
| except ImportError: | ||
| self.SparseObservable = None | ||
|
|
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.
When running the test locally, do you need this try-except for the import? If qiskit is installed we shouldn't need this and just have a global
from qiskit.quantum_info import SparseObservableimport
| operators, parameter_prefix = _remove_identities(operators, parameter_prefix) | ||
|
|
||
| if any(op.num_qubits != num_qubits for op in operators): | ||
| # After removing identities, update num_operators to reflect the actual count |
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 could optimize this a bit: first, we can change to update num_operators only inside the if remove_identities statement (otherwise the number of operators won't change). Then, if num_operators is in fact 0 after the update, we can directly return an empty circuit of size num_qubits (also still inside the if-statement). With this we are sure that num_operators > 0 in the rest of the code and we don't have to check that anymore.
| and evolution is None | ||
| and all(isinstance(op, SparsePauliOp) for op in operators) | ||
| and all( | ||
| isinstance(op, SparsePauliOp) or isinstance(op, SparseObservable) |
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.
You can merge two isinstance checks as
| isinstance(op, SparsePauliOp) or isinstance(op, SparseObservable) | |
| isinstance(op, (SparsePauliOp, SparseObservable)) |
(this also occurs somewhere else in the code)
|
|
||
| cleaned_ops = [op for i, op in enumerate(operators) if i not in identity_ops] | ||
| cleaned_prefix = [prefix for i, prefix in enumerate(prefixes) if i not in identity_ops] | ||
| # Handle both string and list prefixes |
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.
Nice catch! This fixes a bug we had before. It would be nice to backport this fix to the 2.3 series. Would you be up for splitting this fix into a separate PR (including a test) that we merge separately from this? If not no worries, I can do a small follow up to fix it in 2.3.
| cleaned_prefix = [prefix for i, prefix in enumerate(prefixes) if i not in identity_ops] | ||
|
|
||
| return cleaned_ops, cleaned_prefix | ||
| return cleaned_ops, cleaned_prefix |
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.
Could we leave the final empty lines in place? 🙂
| self.assertTrue(all("theta" in name for name in param_names)) | ||
|
|
||
|
|
||
| class TestEvolvedOperatorAnsatzSparseObservable(QiskitTestCase): |
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 really need a new test class for this, could you just add the functions to the existing test class above?
| self.assertEqual(ansatz.num_qubits, 1) | ||
| self.assertEqual(ansatz.num_parameters, 1) | ||
|
|
||
| def test_sparse_observable_identity_detection(self): |
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.
I think we can remove this test, we don't usually test internal, private functions since they are subject to change and instead focus on testing the user API. Since you have other tests that check identities are being removed, I would say it's safe to say the internal function works
PR Summary: Add SparseObservable Support to Evolved Operator Ansatz
Overview
Adds support for SparseObservable in evolved_operator_ansatz, enabling the Rust-accelerated path for better performance.
Changes
evolved_operator_ansatz.py:
test_evolved_op_ansatz.py: