Removed Deprecated Pulse Call Instruction by sbrandhsn · Pull Request #11537 · Qiskit/qiskit

sbrandhsn · 2024-01-10T17:42:07Z

Summary

Removed pulse call instruction deprecated in #10247 and #10103

Details and comments

I was a bit unsure about removing all test cases that use pulse.Call since it seemed that pulse.Call sometimes only provides reference values for some of these, but proceeded after reading the discussion in #10103. I hope that was okay.

But just to be sure, it would be great if you (maybe @nkanazawa1989 ? :-) ) can check that all removed test cases are indeed included in https://github.com/Qiskit/qiskit/blob/main/test/python/pulse/test_builder.py or https://github.com/Qiskit/qiskit/blob/main/test/python/pulse/test_builder_v2.py.
I'm especially sceptical that everything in TestParameterGetter and TestParameterSetter could be removed -- there, pulse.Call was used to create some reference values.

qiskit-bot · 2024-01-10T17:42:12Z

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@mtreinish
@nkanazawa1989

coveralls · 2024-01-10T18:17:57Z

Pull Request Test Coverage Report for Build 7532353892

Warning: 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.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.09%) to 89.387%

Totals
Change from base Build 7490243021:	0.09%
Covered Lines:	59429
Relevant Lines:	66485

💛 - Coveralls

…/Qiskit into deprecation-0.25-pulse

ElePT

The removal procedure looks well executed :), I think I don't have enough pulse expertise to judge if any additional unit test is necessary to compensate for the removal of Call.

ElePT · 2024-01-15T12:36:29Z

 from qiskit.test import QiskitTestCase


-class ParameterTestBase(QiskitTestCase):


I haven't taken the time to analyze the test in depth, but would it make sense to keep it and only delete/replace the schedule.Call line?

Thanks for taking a look! :) Yes, there are a couple of test cases that could qualify for removal or rephrasing. The discussion in issue #10103 lead me to believe there are already alternatives for test cases using Pulse.Call, allowing for current Pulse.Call test cases to be removed... Do you suggest to rephrase all test cases using builder.call instead of Pulse.Call?

You shouldn't remove these tests. This file covers the test cases for schedule parameter assignment.

nkanazawa1989

Thanks @sbrandhsn for taking care of this. I really appreciate. My comment is largely about these two:

You can revert changes to symbolic pulse, since this is done by #11403
Please do NOT delete the entire parameter assignment test test.python.pulse.test_parameter_manager. You can just remove dependency on Call from the reference object.

nkanazawa1989 · 2024-01-15T15:40:11Z

 from qiskit.test import QiskitTestCase


-class ParameterTestBase(QiskitTestCase):


You shouldn't remove these tests. This file covers the test cases for schedule parameter assignment.

nkanazawa1989 · 2024-01-15T15:52:38Z

-
-        self.assertEqual(assigned, ref_obj)
-
-    def test_nested_assignment_partial_bind(self):


Please keep this test. You can just remove dependency on Call and add subroutine directly here because subroutine is ScheduleBlock.

…e1c768.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

sbrandhsn · 2024-01-15T17:44:43Z

Arigatou gozaimashita, @nkanazawa1989 ! :-) I was not familiar with the pulse part of qiskit, thanks for clarifying the deprecation of Pulse.Call!

This PR also removes:

test_parameters_from_subroutine
test_assign_parameter_to_subroutine
test_assign_parameter_to_subroutine_parameter

nkanazawa1989

@sbrandhsn Thanks for quick update!

* removed deprecated pulse.Call * up reno right location * doc fix * fixes qpy error * fix temporary files * updated release note * Update releasenotes/notes/deprecate-pulse-instruction-call-52dca0dd26e1c768.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * reintroduced testcases that used Pulse.Call --------- Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

removed deprecated pulse.Call

311ea2b

sbrandhsn requested a review from nkanazawa1989 January 10, 2024 17:42

sbrandhsn requested review from a team, eggerdj and wshanks as code owners January 10, 2024 17:42

sbrandhsn added the Changelog: Removed Add a "Removed" entry in the GitHub Release changelog. label Jan 10, 2024

up reno right location

c8278e5

sbrandhsn added 7 commits January 11, 2024 01:50

Merge branch 'main' into deprecation-0.25-pulse

4da9a0d

doc fix

67c426f

Merge branch 'deprecation-0.25-pulse' of https://github.com/sbrandhsn…

05333a9

…/Qiskit into deprecation-0.25-pulse

fixes qpy error

5167a8f

fix temporary files

e581ed4

Merge branch 'main' into deprecation-0.25-pulse

f5e78ba

updated release note

b730a35

ElePT reviewed Jan 15, 2024

View reviewed changes

ElePT added this to the 1.0.0 milestone Jan 15, 2024

nkanazawa1989 suggested changes Jan 15, 2024

View reviewed changes

nkanazawa1989 added the mod: pulse Related to the Pulse module label Jan 15, 2024

sbrandhsn and others added 2 commits January 15, 2024 17:41

Update releasenotes/notes/deprecate-pulse-instruction-call-52dca0dd26…

0d74e67

…e1c768.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

reintroduced testcases that used Pulse.Call

5a1730b

sbrandhsn requested a review from nkanazawa1989 January 15, 2024 19:04

nkanazawa1989 approved these changes Jan 16, 2024

View reviewed changes

nkanazawa1989 added this pull request to the merge queue Jan 16, 2024

Merged via the queue into Qiskit:main with commit 0a860fd Jan 16, 2024

		from qiskit.test import QiskitTestCase


		class ParameterTestBase(QiskitTestCase):


		self.assertEqual(assigned, ref_obj)

		def test_nested_assignment_partial_bind(self):

Conversation

sbrandhsn commented Jan 10, 2024

Summary

Details and comments

Uh oh!

qiskit-bot commented Jan 10, 2024

Uh oh!

coveralls commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 7532353892

Warning: This coverage report may be inaccurate.

💛 - Coveralls

Uh oh!

ElePT left a comment

Choose a reason for hiding this comment

Uh oh!

ElePT Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

sbrandhsn Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkanazawa1989 Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nkanazawa1989 Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbrandhsn commented Jan 15, 2024

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jan 10, 2024 •

edited

Loading