-
Notifications
You must be signed in to change notification settings - Fork 209
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
Speed-up QubitMapper.mode_based_mapping
#397
Conversation
Add a benchmark of SparsePauliOp._add via SparsePauliOp.__add__. This method is an important building block of various algorithms (I'm working on the qubit mapper of Qiskit nature). I'm working on the performance improvement of this method. I hope the improvement will be visible. Qiskit/qiskit#7138 qiskit-community/qiskit-nature#397 * Add a benchmark of SparsePauliOp.add * use `__add__` * tweak param list
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 @t-imamichi!
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 LGTM! Thanks @t-imamichi
Because Qiskit/qiskit#7202 is merged, I replace the workaround of |
a462c4c
to
98b09d1
Compare
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.
LGTM!
@t-imamichi we can merge this when you can enable Allow edit by maintainers
so that we can keep this branch updated with main
👍
Thanks. I forgot to enable it. I did it now. |
@woodsp-ibm Offline you mentioned that you might have some more comments on this PR. Is that still the case or can this be merged? |
* Speedup QubitMapper.mode_based_mapping * update comments and fix lint * replace `_sum` with SparsePauliOp.sum Co-authored-by: Max Rossmannek <oss@zurich.ibm.com> Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
…#1348) Add a benchmark of SparsePauliOp._add via SparsePauliOp.__add__. This method is an important building block of various algorithms (I'm working on the qubit mapper of Qiskit nature). I'm working on the performance improvement of this method. I hope the improvement will be visible. Qiskit#7138 qiskit-community/qiskit-nature#397 * Add a benchmark of SparsePauliOp.add * use `__add__` * tweak param list
…#1348) Add a benchmark of SparsePauliOp._add via SparsePauliOp.__add__. This method is an important building block of various algorithms (I'm working on the qubit mapper of Qiskit nature). I'm working on the performance improvement of this method. I hope the improvement will be visible. Qiskit#7138 qiskit-community/qiskit-nature#397 * Add a benchmark of SparsePauliOp.add * use `__add__` * tweak param list
…#1348) Add a benchmark of SparsePauliOp._add via SparsePauliOp.__add__. This method is an important building block of various algorithms (I'm working on the qubit mapper of Qiskit nature). I'm working on the performance improvement of this method. I hope the improvement will be visible. Qiskit#7138 qiskit-community/qiskit-nature#397 * Add a benchmark of SparsePauliOp.add * use `__add__` * tweak param list
Summary
This PR improves performance of JW transformation by
SparsePauliOp
of "+", "-", "E", "N" beforehandSparsePauliOp._add
If we introduce a specialized function to compute the sum of
SparsePauliOp
(e.g.,SparsePauliOp.sum(list[SparsePauliOp]) -> SparsePauliOp
, we can further improve the performance around 2x. But, we need discussion to introduce a new public API to Terra.Update:
SpasePauliOp.sum
is merged Qiskit/qiskit#7202. So, I replaced the workaround of sum withSparsePauliOp.sum
.Details and comments
I also prepared separate PRs to improve the performance of JW as follows:
SparsePauliOp.simplify
Qiskit/qiskit#7122SparsePauliOp.compose
Qiskit/qiskit#7126SparsePauliOp._add
Qiskit/qiskit#7138I notice that
SparsePauliOp.simplify
has a critical performance issue, which is addressed at Qiskit/qiskit#7122, so, this PR with terra main still takes a long time.Microbenchmark
Update: the final version with SparsePauliOp.sum takes 1.63 sec.