Check for gate parameter #12439

MozammilQ · 2024-05-18T01:41:40Z

Summary

This PR aims to fix an issue withTarget.has_calibration() and Target.get_calibration() when passed with parameterized gates didn't work as expected.

Refer to issues for more information.
#11658
#11657

Details and comments

To be precise, Target.has_calibration() didn't check if the calibration if present is present for a particular gate with a particular parameter,
and,
Target.get_calibration() didn't get the calibration for the gate if present is present for a particular gate with a particular parameter, when passed as a parameter as args, the method tried to bind this args with a parameter in the schedule, which was not the intention of this particular args.

So, the signature of Target.has_calibration includes a new argument operation_params which has to be passed with the parameter of the Gate if the Gate is parameterized.

Similarly, the signature of Target.get_calibration also includes a new argument operation_params which has to be passed with the parameter of the Gate if the Gate is parameterized. In the case of args argument in this function, it should be passed with the values to be bound with the parameter of the schedule which has been attached to this particular gate as its calibration.

fixes #11658
fixes #11657

qiskit-bot · 2024-05-18T01:41:46Z

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 following people are relevant to this code:

@Qiskit/terra-core
@nkanazawa1989

coveralls · 2024-05-18T02:00:10Z

Pull Request Test Coverage Report for Build 9136334258

Details

6 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.006%) to 89.607%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	4	92.62%

Totals
Change from base Build 9134495129:	0.006%
Covered Lines:	62302
Relevant Lines:	69528

💛 - Coveralls

MozammilQ · 2024-05-19T00:20:45Z

qiskit/transpiler/passes/calibration/pulse_gate.py

@@ -95,4 +95,4 @@ def get_calibration(self, node_op: CircuitInst, qubits: List) -> Union[Schedule,
        Raises:
            TranspilerError: When node is parameterized and calibration is raw schedule object.
        """
-        return self.target.get_calibration(node_op.name, tuple(qubits), *node_op.params)
+        return self.target.get_calibration(node_op.name, tuple(qubits), None, *node_op.params)


Reviewer's comment anticipated! :)
I am not super satisfied with putting None here.
Here, the parameter node_op.params has the actual list of float parameters to bind with the Gate's parameters stored in the target which is a parameter(variable). That is why I had to put None just to keep the function's behavior similar to its previous version.

MozammilQ · 2024-05-19T00:24:14Z

test/python/transpiler/test_target.py

+        )
+
+    def test_get_calibration_for_oper_with_param_pulse_with_param(self):
+        args = {0.23, 0.123, 0.2}


Considering theta maps to amp, sigma, and, beta. The emphasis here is just to prove that multiple *args could be successfully bind if equal number of parameter(variable) present in the actual Schedule

MozammilQ · 2024-10-29T23:10:12Z

@1ucian0 , pulse features are being deprecated in Qiskit, and being moved to Qiskit Dynamics, is this PR required anymore?
If yes, in what direction the PR should be heading?

eliarbel

Hi @MozammilQ , thanks for working on this! Yes, although Pulse is being deprecated in Qiskit 1.3 and will be removed in 2.0, it would still be good to have this as part of the extended support window for the 1.x series.

For moving forward please note the following:

Update your branch to include all the recent changes including the move Target to Rust. We still have Target class in target.py with has_calibration and get_calibration there so your changes are still valid.
Once this PR is ready we will want to merge it into the 1.4 branch and not 2.0.

I also left some line comments after partially reviewing this.

eliarbel · 2024-11-20T11:20:54Z

qiskit/transpiler/target.py

    ) -> bool:
        """Return whether the instruction (operation + qubits) defines a calibration.

        Args:
            operation_name: The name of the operation for the instruction.
            qargs: The tuple of qubit indices for the instruction.
+            operation_params: The parameters for the Instruction.
+                              Even for a single parameter, provide as a list of float


We can omit this refinement as the type hint explicitly specs a list param and the code converts to list from a scalar anyway.

eliarbel · 2024-11-20T11:21:38Z

qiskit/transpiler/target.py

@@ -907,15 +923,25 @@ def get_calibration(
        Args:
            operation_name: The name of the operation for the instruction.
            qargs: The tuple of qubit indices for the instruction.
+            operation_name: The parameters for the instruction.


Duplicate entry for operation_name, need to be removed

eliarbel · 2024-11-20T11:22:27Z

qiskit/transpiler/target.py

@@ -907,15 +923,25 @@ def get_calibration(
        Args:
            operation_name: The name of the operation for the instruction.
            qargs: The tuple of qubit indices for the instruction.
+            operation_name: The parameters for the instruction.
+            operation_params: The parameters for the Instruction.
+                            Even for a single parameter, provide as a list of float


Same comment as in has_calibration

eliarbel · 2024-11-20T11:25:21Z

releasenotes/notes/Fixed-param-gate-target-has-and-get_calibration-d2b9e38d63592b3c.yaml

+---
+fixes:
+  - |
+    Fixes an issue with :func:`Target.has_calibration` and :func:`Target.get_calibration` 


Suggested change

Fixes an issue with :func:`Target.has_calibration` and :func:`Target.get_calibration`

Fixed an issue with :func:`Target.has_calibration` and :func:`Target.get_calibration`

eliarbel · 2024-11-20T11:25:32Z

releasenotes/notes/Fixed-param-gate-target-has-and-get_calibration-d2b9e38d63592b3c.yaml

+  - |
+    Fixes an issue with :func:`Target.has_calibration` and :func:`Target.get_calibration` 
+    when passed with a parameterized Gate doesn't work as expected.
+    Refer to `qiskit/#11657 <https://github.com/Qiskit/qiskit/issues/11657>` and, 


Suggested change

Refer to `qiskit/#11657 <https://github.com/Qiskit/qiskit/issues/11657>` and,

Refer to `qiskit/#11657 <https://github.com/Qiskit/qiskit/issues/11657>`__ and,

eliarbel · 2024-11-20T11:25:49Z

releasenotes/notes/Fixed-param-gate-target-has-and-get_calibration-d2b9e38d63592b3c.yaml

+    Fixes an issue with :func:`Target.has_calibration` and :func:`Target.get_calibration` 
+    when passed with a parameterized Gate doesn't work as expected.
+    Refer to `qiskit/#11657 <https://github.com/Qiskit/qiskit/issues/11657>` and, 
+    `qiskit/#11658 <https://github.com/Qiskit/qiskit/issues/11658>` for more information.


Suggested change

`qiskit/#11658 <https://github.com/Qiskit/qiskit/issues/11658>` for more information.

`qiskit/#11658 <https://github.com/Qiskit/qiskit/issues/11658>` __ for more information.

eliarbel · 2024-11-20T11:38:48Z

qiskit/transpiler/target.py

    def get_calibration(
        self,
        operation_name: str,
        qargs: tuple[int, ...],
+        operation_params: list[float] | None = None,


Why not ordering like this?

def get_calibration( self, operation_name: str, qargs: tuple[int, ...], *args: ParameterValueType, operation_params: list[float] | None = None, **kwargs: ParameterValueType, ) -> Schedule | ScheduleBlock:

MozammilQ added 5 commits May 16, 2024 23:54

added args to both methods

07ebaa6

refactored code; lint

7236750

added test

af66a2d

added release note

3f7f5ff

lint

c817559

MozammilQ requested a review from a team as a code owner May 18, 2024 01:41

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label May 18, 2024

MozammilQ commented May 19, 2024

View reviewed changes

MozammilQ changed the title ~~[WIP] Check for gate parameter~~ Check for gate parameter May 19, 2024

eliarbel requested changes Nov 20, 2024

View reviewed changes

Merge branch 'main' into check-oper-param

8119c77

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for gate parameter #12439

Check for gate parameter #12439

MozammilQ commented May 18, 2024 •

edited

Loading

qiskit-bot commented May 18, 2024

coveralls commented May 18, 2024

MozammilQ May 19, 2024

MozammilQ May 19, 2024

MozammilQ commented Oct 29, 2024

eliarbel left a comment

eliarbel Nov 20, 2024

eliarbel Nov 20, 2024

eliarbel Nov 20, 2024

eliarbel Nov 20, 2024

eliarbel Nov 20, 2024

eliarbel Nov 20, 2024

eliarbel Nov 20, 2024

	Fixes an issue with :func:`Target.has_calibration` and :func:`Target.get_calibration`
	Fixed an issue with :func:`Target.has_calibration` and :func:`Target.get_calibration`

	Refer to `qiskit/#11657 <https://github.com/Qiskit/qiskit/issues/11657>` and,
	Refer to `qiskit/#11657 <https://github.com/Qiskit/qiskit/issues/11657>`__ and,

	`qiskit/#11658 <https://github.com/Qiskit/qiskit/issues/11658>` for more information.
	`qiskit/#11658 <https://github.com/Qiskit/qiskit/issues/11658>` __ for more information.

Check for gate parameter #12439

Are you sure you want to change the base?

Check for gate parameter #12439

Conversation

MozammilQ commented May 18, 2024 • edited Loading

Summary

Details and comments

qiskit-bot commented May 18, 2024

coveralls commented May 18, 2024

Pull Request Test Coverage Report for Build 9136334258

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MozammilQ commented Oct 29, 2024

eliarbel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MozammilQ commented May 18, 2024 •

edited

Loading