-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Remove deprecated methods and args in Primitives #9480
Remove deprecated methods and args in Primitives #9480
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:
|
`BaseSampler` and `BaseEstimator` in terra deprecated circuits, observables and parameters as arguments for their constructors. They will be reduced in the next release of terra (0.24). Because it took three months since terra started warning of deprecation for these arguments, Aer reduces them from their arguments before terra's PR (Qiskit/qiskit#9480) is released. * Remove deprecated methods and args in Primitives * add reno Co-authored-by: Hiroshi Horii <[email protected]>
@@ -239,8 +189,7 @@ def run( | |||
**run_opts.__dict__, | |||
) | |||
|
|||
# This will be comment out after 0.22. (This is necessary for the compatibility.) | |||
# @abstractmethod | |||
@abstractmethod |
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 am not sure if you want to do anything about the raise in the body here (same in base_sampler) - python now will not let you instantiate any class without it implemented right so the only time you might see this is looking in this source code if some subclass you created was failing and you had not dealt with the deprecation yet. So if its more documenting that this was changed to abstract that did not (will not) happen until 0.24 right - not 0.22.
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 am late in removing my comment because of the delay in updating the other libraries. Well, 0.24 would also be after 0.22.
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.
Seems like we need to at least update the error message in the NotImplementedError
here, correct? It mentions 0.22
.
This method can still be called by a concrete subclass, so IMO we should still raise NotImplementedError
with a more suitable message.
super().__init__(options) | ||
|
||
################################################################################ |
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 don't think this removal is related to this deprecation. I'm not a fan of this type of comment to indicate sections, but we need to discuss the removal carefully.
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.
Yeah. This is my bad. This was mistakenly approved during the review at the time of introduction.
Let's discuss here.
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.
Is there more discussion needed here? Personally, it seems like the section headings are less useful now that many of the things under Deprecations
have been removed.
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.
Hi @ikkoham! Looks good so far.
If you have time, please address these comments so we can get this merged before the upcoming release.
super().__init__(options) | ||
|
||
################################################################################ |
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.
Is there more discussion needed here? Personally, it seems like the section headings are less useful now that many of the things under Deprecations
have been removed.
@@ -239,8 +189,7 @@ def run( | |||
**run_opts.__dict__, | |||
) | |||
|
|||
# This will be comment out after 0.22. (This is necessary for the compatibility.) | |||
# @abstractmethod | |||
@abstractmethod |
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.
Seems like we need to at least update the error message in the NotImplementedError
here, correct? It mentions 0.22
.
This method can still be called by a concrete subclass, so IMO we should still raise NotImplementedError
with a more suitable message.
releasenotes/notes/delete-args-and-methods-in-primitives-d89d444ec0217ae6.yaml
Outdated
Show resolved
Hide resolved
@@ -58,358 +58,6 @@ def setUp(self): | |||
[1, 2, 3, 4, 5, 6], | |||
) | |||
|
|||
def test_estimator(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.
Just to double check, is there anything we're removing here that should be ported to use whatever the proper new code paths are? In other words, are we inadvertently reducing code coverage?
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.
Thank you. I checked all tests. I readded three tests: test_run_with_operator
and test_run_with_shots_option_none
for Estimator and test_run_reverse_meas_order
for Sampler.
- I removed duplicated tests.
- We don't test e2e test for PauliSumOp, instead we added the conversion test (
test_validate_observables
). - No tests corresponding to
test_estimator_param_reverse
since we do not have this feature (Passing parameters, notparameter_values
).
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.
@kevinhartman Thank you very much for your review. I improved the code now. Could you check again, please?
@@ -58,358 +58,6 @@ def setUp(self): | |||
[1, 2, 3, 4, 5, 6], | |||
) | |||
|
|||
def test_estimator(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.
Thank you. I checked all tests. I readded three tests: test_run_with_operator
and test_run_with_shots_option_none
for Estimator and test_run_reverse_meas_order
for Sampler.
- I removed duplicated tests.
- We don't test e2e test for PauliSumOp, instead we added the conversion test (
test_validate_observables
). - No tests corresponding to
test_estimator_param_reverse
since we do not have this feature (Passing parameters, notparameter_values
).
I added the label |
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
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!
LGTM
* Remove deprecated methods and args in Primitives * rm unnecessary comments * add reno * Update qiskit/primitives/base/base_sampler.py * From review comments
* Remove deprecated methods and args in Primitives * rm unnecessary comments * add reno * Update qiskit/primitives/base/base_sampler.py * From review comments
* Remove deprecated methods and args in Primitives * rm unnecessary comments * add reno * Update qiskit/primitives/base/base_sampler.py * From review comments
* Remove deprecated methods and args in Primitives * rm unnecessary comments * add reno * Update qiskit/primitives/base/base_sampler.py * From review comments
Summary
Remove methods and args in primitives deprecated from Terra 0.22.
Details and comments