Skip to content

Remove deprecation for pulse instruction parameter method#6277

Merged
mergify[bot] merged 10 commits into
Qiskit:mainfrom
nkanazawa1989:remove_parameter_deprecation
Apr 26, 2021
Merged

Remove deprecation for pulse instruction parameter method#6277
mergify[bot] merged 10 commits into
Qiskit:mainfrom
nkanazawa1989:remove_parameter_deprecation

Conversation

@nkanazawa1989
Copy link
Copy Markdown
Contributor

Summary

The parameter manager (in PR #5854) deprecated the instruction-level management of parameters to realize light-weight instruction object, i.e.

inst = Play(Gaussian(**params), d0)
inst.parameters  # deprecated

However this method is still needed for writing calibration code (thanks @eggerdj !). This PR removes deprecation warnings and add new logic to get the parameter set without initializing parameter table in the constructor.

Details and comments

No unittest is added by this PR, because there are tests for .parameters method in test/python/pulse/test_parameters.py. These tests were not removed by #5854 for backward compatibility thus they should be still valid.

@mtreinish
Copy link
Copy Markdown
Member

Can you add a release note here? Probably an other note saying this isn't deprecated anymore. Since IIRC there was release note about this deprecation, we should document we're reversing that and it's not deprecated anymore and will continue to be supported.

@nkanazawa1989
Copy link
Copy Markdown
Contributor Author

Thanks @mtreinish for catching this. Yes, deprecation node was added to 0.17 release. Reno is added.

eggerdj
eggerdj previously approved these changes Apr 22, 2021
Copy link
Copy Markdown
Contributor

@eggerdj eggerdj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@kdk kdk added the automerge label Apr 22, 2021
for pulse_param in pulse_param_expr.parameters:
parameters.add(pulse_param)
if self.channel.is_parameterized():
for ch_param in self.channel.parameters:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the tests need to be extended with parametrised channels: https://coveralls.io/builds/39061954/source?filename=qiskit%2Fpulse%2Finstructions%2Fplay.py#L105

Copy link
Copy Markdown
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fully sure, but maybe a new test is needed to cover the added code.

@nkanazawa1989
Copy link
Copy Markdown
Contributor Author

Thanks @1ucian0 for catching this. Though these functionalities have been existing, it seems like we didn't have specific test for them. I added new test for play instruction paramters.

@taalexander taalexander requested a review from 1ucian0 April 26, 2021 14:20
Copy link
Copy Markdown
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mergify mergify Bot merged commit 71d98f8 into Qiskit:main Apr 26, 2021
@1ucian0 1ucian0 added the Changelog: Removed Add a "Removed" entry in the GitHub Release changelog. label Jun 25, 2021
@mtreinish mtreinish added Changelog: Changed Add a "Changed" entry in the GitHub Release changelog. and removed Changelog: Removed Add a "Removed" entry in the GitHub Release changelog. labels Jun 30, 2021
@nkanazawa1989 nkanazawa1989 deleted the remove_parameter_deprecation branch November 25, 2022 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Changed Add a "Changed" entry in the GitHub Release changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants