Remove pulse scoped_parameters and search_parameters #11692
Conversation
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7736372337Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
wshanks
added
the
Changelog: Removal
The `scoped_parameters` and `search_parameters` methods have been removed from the `ScheduleBlock` class. These methods returned `Parameter` objects that partially linked to the parameters in the `ScheduleBlock` instance but assigning values using these objects did not work correctly. Users should use `ScheduleBlock.parameters` instead and iterate through `ScheduleBlock.references` and compare to the `Schedule.parameters` attributes of the subreferences when needing to distinguish which subroutine a parameter is used in. See Qiskit#11654 for more information.
wshanks
force-pushed
the
remove-parameter-scoping
branch
from
7676fba to
5999251
Compare
There was a problem hiding this comment.
I guess my main comments are really #11691 (review) and #11691 (comment).
The scale of the necessary documentation changes here worries me that these methods might be more widely used, and their removal maybe shouldn't be rushed in, but I don't know pulse users, and I'll defer to @wshanks and @nkanazawa1989 as code-owners - there is of course the giant mitigating circumstance that these methods probably didn't work for their intended use case before (which tbh is not great that we didn't spot...).
Other than that, the removal looks generally ok, and it's good to increase the test coverage for the actual assignment, thanks Will.
| The ``scoped_parameters`` and ``search_parameters`` methods have been | ||
| removed from the `.ScheduleBlock` class. These methods returned | ||
| `.Parameter` objects that partially linked to the parameters in the | ||
| `.ScheduleBlock` instance but assigning values using these objects did not | ||
| work correctly. Users should use `.ScheduleBlock.parameters` instead and | ||
| iterate through `.ScheduleBlock.references` and compare to the | ||
| `.Schedule.parameters` attributes of the subreferences when needing to | ||
| distinguish which subroutine a parameter is used in. See `#11654 | ||
| https://github.com/Qiskit/qiskit/issues/11654`__ for more information. |
There was a problem hiding this comment.
| The ``scoped_parameters`` and ``search_parameters`` methods have been | |
| removed from the `.ScheduleBlock` class. These methods returned | |
| `.Parameter` objects that partially linked to the parameters in the | |
| `.ScheduleBlock` instance but assigning values using these objects did not | |
| work correctly. Users should use `.ScheduleBlock.parameters` instead and | |
| iterate through `.ScheduleBlock.references` and compare to the | |
| `.Schedule.parameters` attributes of the subreferences when needing to | |
| distinguish which subroutine a parameter is used in. See `#11654 | |
| https://github.com/Qiskit/qiskit/issues/11654`__ for more information. | |
| The ``scoped_parameters`` and ``search_parameters`` methods have been | |
| removed from the :class:`.ScheduleBlock` class. These methods returned | |
| :class:`.Parameter` objects that partially linked to the parameters in the | |
| :class:`.ScheduleBlock` instance but assigning values using these objects did not | |
| work correctly. Users should use :attr:`.ScheduleBlock.parameters` instead and | |
| iterate through :attr`.ScheduleBlock.references` and compare to the | |
| :attr:`.Schedule.parameters` attributes of the subreferences when needing to | |
| distinguish which subroutine a parameter is used in. See `#11654 | |
| <https://github.com/Qiskit/qiskit/issues/11654>`__ for more information. |
qiskit/pulse/schedule.py
Outdated
There was a problem hiding this comment.
Lint is just complaining about a now-unused import re at the top of this file, I think.
There was a problem hiding this comment.
This looks ok to me. I'll approve and tag for merge to get things moving, but I'm not knowledgeable about the pulse module, so I don't know much about the documentation. If another pulse maintainer spots problems in it, let's open a new PR after 1.0.0rc1 is out to correct the documentation.
The
scoped_parametersandsearch_parametersmethods have been removed from theScheduleBlockclass. These methods returnedParameterobjects that partially linked to the parameters in theScheduleBlockinstance but assigning values using these objects did not work correctly. Users should useScheduleBlock.parametersinstead and iterate throughScheduleBlock.referencesand compare to theSchedule.parametersattributes of the subreferences when needing to distinguish which subroutine a parameter is used in. See #11654 for more information.The code removal itself was straightforward here because the relevant methods were not used anywhere else except the tests. For the tests, it was a little tricky because I didn't want to remove logic that was testing schedule references. Where I could, I changed the scoped parameter checks to regular parameter checks. I might have kept more tests than needed. I added one test for assignment to a parameter in a subroutine since not having such a test was why #11654 was not found earlier.
The documentation was also tricky because the scoped parameters were used to document some of the features of schedule references. I tried to pull out the discussion of scoped parameters while leaving the examples of assigning references. I substituted in some use of regular parameters (changed to have distinguishable names) just as a way to show that data from the subroutine was available in the caller after the assignment.
Related deprecation in #11691.