Deprecate pulse parameter scoping #11691
Conversation
As noted in Qiskit#11654, parameter scoping does not work properly beginning with Qiskit 0.45.0. The problem is that the `Parameter` name is now used for the hash value and so the parameter produced with the scoped name is not equivalent to the original parameter and does not substitute for it when using parameter assignment. Since the scoped parameter mechanism was a convenience feature and not widely used, it is simply deprecated rather than made to work with parameter assignment.
|
One or more of the the following people are requested to review this:
|
|
I think this might be well past the deadline for adding deprecations to 0.46? I hope it can still make it as an exception because it is a sort of bug fix as well. It adds a warning to a feature that already is not working correctly and which we plan to remove rather than fix. |
There was a problem hiding this comment.
LGTM, thanks @wshanks for the quick update before the deadline.
wshanks
added
Changelog: Deprecation
There was a problem hiding this comment.
You're right that this is way past the freeze deadline (and the removal one is similarly past the deadline for 1.0), but given that the deprecation and removal look to be fairly clean, and as you say, they're for features that don't really work correctly and are preventing an absolutely necessary bugfix (#11652), I'll accept them if you and Naoki are sure that these methods aren't used by any pulse code we're aware of.
edit: the point being that I'm not very aware of the pulse ecosystem and use, so I don't know how it's used.
qiskit/pulse/schedule.py
Outdated
| warnings.warn( | ||
| "Scoped parameters may not work correctly with parameter assignment.", | ||
| stacklevel=3, | ||
| ) |
There was a problem hiding this comment.
I think we can skip this - it's not really user actionable, and it's in a private function that's only used by deprecated functions.
There was a problem hiding this comment.
I used stacklevel 3 here as this private function is used by both public functions that return scoped parameters. Maybe it's better practice to put the warning in each public function directly. In any case, the plan is to remove the two public functions and this private function together as soon as permissible.
There was a problem hiding this comment.
This does mean that the methods produce both a user warning and a deprecation warning, but it felt like squeezing too much in to add this text about the current behavior not working correctly into the deprecation warning .
There was a problem hiding this comment.
Given the function is deprecated, I think it'd be better to move the warning message into the methods' docstring and not issue an actual warning which will trigger on every call. In the interests of moving forwards on release day, I'll just push that up and merge the PR.
releasenotes/notes/deprecate-pulse-parameter-scoping-6b6b50a394b57937.yaml
Outdated
Show resolved
Hide resolved
|
I guess a better way of setting the acceptance criteria would be: was it possible to write a useful program using scoped parameters? Could a user have actually done meaningful things with them? My approximate understanding is "no", because assignment wouldn't work due to hash-map lookups failing, and that's my reason for accepting the PR - if there are useful things that could have been done, I think it's a bit more questionable. The scope of the documentation change in #11692 makes me a bit more nervous. |
…4b57937.yaml Co-authored-by: Jake Lishman <[email protected]>
Yes, I had hoped to come to a resolution in #11654 a little more quickly but even that was past the deadline. Personally, I don't have a pending feature relying on this removal or on #11652. My only goal was to keep known bugs out of new releases, but they aren't critical bugs -- they are in 0.45 and no one has complained -- so if we need to push them off to the next release that is okay. I was just giving best effort to get the fixes in.
Right, scoped parameters are mostly broken in 0.45, so I don't think anyone will be affected by their removal. @nkanazawa1989 and I don't know of anyone using them, and my GitHub-wide searches for usage didn't find anything. Nested/scoped pulse schedules in general are something we set up and planned to start using but so far hadn't gotten to using. The one thing that I do find the removed methods useful for is schedule introspection in the REPL (printing out where parameters are used). This usage is not affected by the assignment bug and is part of why I wondered about changing the methods to return dicts of It sounds like we are okay with merging all the PR's. If we had to we could just mark the test failures in #11652 as expected failures and push this PR to the next release. It would be nice to be able to remove the code in 1.0 though. |
As noted in #11654, parameter scoping does not work properly beginning with Qiskit 0.45.0. The problem is that the
Parametername is now used for the hash value and so the parameter produced with the scoped name is not equivalent to the original parameter and does not substitute for it when using parameter assignment. Since the scoped parameter mechanism was a convenience feature and not widely used, it is simply deprecated rather than made to work with parameter assignment.Related removal in #11692.