Add run method to primitives #8382
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 2821812085
💛 - Coveralls |
|
I'm wondering which is appropriate, |
|
@t-imamichi If my understanding is correct, |
asyncio.Future would be fine as is, except it requires the user to use |
|
@rathishcholarajan suggested somewhere that we should use |
ikkoham
changed the title
|
@rathishcholarajan @jyu00 Thank you for your review. I improved codes according to your comments. |
|
LGTM. I have just a few minor comments. |
| index = self._circuit_ids.get(id(circuit)) | ||
| if index is not None: | ||
| circuit_indices.append(index) | ||
| else: | ||
| circuit_indices.append(len(self._circuits)) | ||
| self._circuit_ids[id(circuit)] = len(self._circuits) | ||
| self._circuits.append(circuit) |
There was a problem hiding this comment.
Personally I'd use an OrderedDict that has the circuit ids as the keys and circuits as the values. This allowed a single variable instead of having to have both _circuits and _circuit_ids. AnOrderedDict also allows accessing by index (e.g. od.values()[0] to support the deprecated circuit indexes.
The down side is you'd probably have to refactor the _call method.
There was a problem hiding this comment.
Now dict is ordered, so we can simply use dict. But it takes O(n) to find index.
In the future (after deprecation of __call__), we don't need ids, so I want to refactor after that.
Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
| def close(self): | ||
| """Close the session and free resources""" |
There was a problem hiding this comment.
I think close() can be deprecated as well, since there is no longer a session associated with a primitive.
There was a problem hiding this comment.
Yes. __exit__ was deprecated instead.
| @@ -184,7 +198,7 @@ def circuits(self) -> tuple[QuantumCircuit, ...]: | |||
| Returns: | |||
| The quantum circuits to be sampled. | |||
| """ | |||
| return self._circuits | |||
| return tuple(self._circuits) | |||
There was a problem hiding this comment.
Now that circuits can be added to each run(), is this method intended to return the concatenated list of all circuits?
There was a problem hiding this comment.
Yes. I'm not sure that we need this new interface. Should we deprecate this?
There was a problem hiding this comment.
I guess wanting to see all circuits you submitted is a plausible use case? We already have to keep the circuits around so keeping this function shouldn't be a big deal.
Co-authored-by: Jessie Yu <jessieyu@us.ibm.com>
|
@jyu00 @t-imamichi Can we merge this PR? or do we have other discussion points? |
|
@mergify refresh |
✅ Pull request refreshed |
Recent releases of Terra and Aer are not synchronized and Terra-0.22 will be released before Aer-0.12 will be released. Primitives in Terra 0.22 have new feature run method. Qiskit/qiskit#8382. This PR makes Aer 0.11 compatible with both Terra 0.21 and 0.22. Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>
Summary
Notebook: https://gist.github.com/ikkoham/8da241f47c2145c1be61704c6ac9193d
Details and comments