-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add fast-path parameter binding #14782
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
Pull Request Test Coverage Report for Build 16468882750Details
💛 - Coveralls |
The new `ParameterExpresion.bind_all` is a fast path for producing a numeric result. This has a couple of advantages over `ParameterExpression.bind`: - we can allocate far fewer Python objects, since we do not need to allocate new versions of all the additional data that goes along with a complete `ParameterExpression` object; we know the output must be numeric. - we have no historical API reasons to scan through the incoming mapping to search for invalid keys or values; this is a huge performance improvement for the case of using the same mapping to bind many different expressions (though there are ways that `ParameterExpression.bind` could be improved and its interface safely evolved to remove the worst cases of the scaling). There is still a sizeable amount of performance gain to be had, because the interfaces in the Rust-space `ParameterExpression` and `SymbolExpr` require rather more heap allocations per binding call than are entirely necessary. That said, this already provides a large performance improvement, and a complexity improvement for the case of a large values dictionary being used many times.
d33a3dc
to
fc2ead8
Compare
One or more of the following people are relevant to this code:
|
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.
Would it make sense to expose a similar bind_all
to the circuit, where we know that all parameters are being bound and we can call this fast path?
And another question actually: what do you think about changing Qiskit's default to not check for parameter existence? Indeed we could be faster and maybe we ought to speed up the default behavior. I think most symbolic engines do not have these existence checks we added.
There's a bunch of ways to reduce the overhead and the unnecessary looping through the parameter dictionaries in |
The new
ParameterExpresion.bind_all
is a fast path for producing a numeric result. This has a couple of advantages overParameterExpression.bind
:we can allocate far fewer Python objects, since we do not need to allocate new versions of all the additional data that goes along with a complete
ParameterExpression
object; we know the output must be numeric.we have no historical API reasons to scan through the incoming mapping to search for invalid keys or values; this is a huge performance improvement for the case of using the same mapping to bind many different expressions (though there are ways that
ParameterExpression.bind
could be improved and its interface safely evolved to remove the worst cases of the scaling).There is still a sizeable amount of performance gain to be had, because the interfaces in the Rust-space
ParameterExpression
andSymbolExpr
require rather more heap allocations per binding call than are entirely necessary. That said, this already provides a large performance improvement, and a complexity improvement for the case of a large values dictionary being used many times.Summary
Details and comments
Fix #14471.
Currently in draft because there's no tests - I'm just putting it up so Sam and Ian from #14471 can test it out for their use case. For the explicit example in that issue, a complete comparison on my machine: