Add allow decorators #8035
Add allow decorators #8035
Conversation
|
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 the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 2300436201
💛 - Coveralls |
|
With this PR, would users have to define a new primitive class to enable e.g. passing circuits as objects? Because that seems like some overhead that's not necessarily obvious... |
|
I would like to rename Why are you going with a decorator approach here? Why not just allow objects to be passed as part of the fundamental interface? |
There was a problem hiding this comment.
Overall I agree with the points raised by @Cryoris and @blakejohnson. I find the decorator syntax and the need of defining a new class very confusing. Can we add these as flags or just directly allow all of these options in the standard interface?
| with CustomEstimator([ansatz], [observable]) as e: | ||
| result = e( | ||
| circuit_indices=[ansatz], | ||
| observable_indices=[observable], | ||
| parameter_values=[parameter], | ||
| ) | ||
| print(result) | ||
|
|
||
| Object equivalency is determined by the object ID. Thus, note that even if they represent the same | ||
| circuit (e.g. ``copy()``), they will not match if they are different objects. | ||
| Also, note that since qiskit's circuit objects and observable objects are not immutable, | ||
| if the object is changed after the construction of the primitives, it may behave unexpectedly. |
There was a problem hiding this comment.
What happens when the object inside the "context" doesn't match with those in the with ... as ...? Since we are passing objects, would it be possible to make it so that we only pass objects inside the "context" like:
with CustomEstimator() as e: # no need to pass anything
result = e(
circuit_indices=[ansatz],
observable_indices=[observable],
parameter_values=[parameter],
)There was a problem hiding this comment.
This extension is not impossible if a circuit update feature is included, but the communication and serialization overhead would be significant.
There was a problem hiding this comment.
I don't quite understand the what you said, probably because I don't understand the whole python context concept. Do you know some resources to understand the context manager better? I am going to read PEP343 where this was introduced.
Do you know any major software that uses the context manager? I would like to see more code examples. The only thing I know is for openning files.
There was a problem hiding this comment.
This is not a context issue, but simply because the cost of circuit serialization and communication is high, so it is faster to communicate a small object like int.
(Major software: for example, requests has Session() that can be used with with.)
|
Thanks. There is an decorator syntaxI essentially only came up with this method. with allow_objects(Estimator)([ansatz], [observable]):
result = e(
circuit_indices=[ansatz],
observable_indieces=[observable],
parameter_values=[parameter],
)It is difficult to modify internal of the estimator class since it is costly for all Runtime services and 3rd parties’ primitives to implement this. naming of
|
|
It just occurred to me that it is indeed possible to implement this on the base class side. If so, the discussion |
|
I personally think that these 3 decorators should be the default behaviour but I can see that the decorator approach is very flexible and easily extendable. Is there other decorators for primitives you have in mind? And Is it possible to use more than one decorator at a time? |
|
After discussing this issue offline with @levbishop and @ajavadia, we decided to make it default instead of custom decorator and migrate the argument names.
Not at the moment, but gradients could be implemented with mixin or decorator. (but now that direction is not a good idea).
Yes, it's possible. But I think in this PR the decorators are orthogonal, but in some cases the order may affect. |
Summary
Allow decorators enhance Primitives (Sampler and Estimator) and provide a more flexible interface.
This PR comes from the discussion: #7723 (comment).
Details and comments