Update target from_configuration by MozammilQ · Pull Request #11461 · Qiskit/qiskit

MozammilQ · 2023-12-27T17:48:21Z

Summary

This PR updates logic of method:from_configuration() of class:Target in qiskit/transpiler/target.py

Credit:
@nkanazawa1989 , this logic is heavily influenced by Qiskit/qiskit-ibm-provider#413
@to24toro , for all the kind help and motivation :)
fixes: #10168

qiskit-bot · 2023-12-27T17:48:27Z

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 the following people are requested to review this:

@Qiskit/terra-core

MozammilQ · 2023-12-27T17:49:54Z

I am done :)

coveralls · 2023-12-29T12:55:07Z

Pull Request Test Coverage Report for Build 8315422102

Details

115 of 131 (87.79%) changed or added relevant lines in 1 file are covered.
12 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.02%) to 89.306%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/target.py	115	131	87.79%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	2	92.7%
qiskit/providers/backend_compat.py	10	80.22%

Totals
Change from base Build 8310563987:	0.02%
Covered Lines:	59825
Relevant Lines:	66989

💛 - Coveralls

MozammilQ · 2024-01-02T09:58:45Z

I am sure this requires some further work, if the code remains the same there has to be some changes in the docstrings.
Probably should add some more tests to increase coverage :)
A review could provide me with better guidance for next steps :)

@nkanazawa1989 , please see if this good enough :)

to24toro

Thanks for your contribution. I have a quick look and have some minor comments.

to24toro · 2023-12-31T13:28:19Z

-            concurrent_measurements=concurrent_measurements,
-        )
-        name_mapping = get_standard_gate_name_mapping()
+        required_operations = {"measure", "delay"}


[Q] I don't still catch up fully, but are any errors detected if the delay operation here is not added.

if 'delay' is removed here,
target does not have delay, because you see,
'delay' is only added when delay is present in 'required_operations'
'delay' is added into inst_name_map through required_operations
then, delay is added to prop_name_map
then finally, delay is added to target through inst_name_map and prop_name_map

To avoid all this hassle,
One, could just write something like this at the very end before return target

if in_data_target['num_qubits'] is not None: target.add_instruction( instruction=qiskit_inst_mapping.get('delay'), properties={(q,): None for q in range(num_qubits) if q not in faulty_qubits}, name='delay' )

My opinion is, it's just a personal preference :)
Naoki , Kento, please correct me if I am missing something :)

nkanazawa1989

Thanks @MozammilQ this is nice improvements. Overall implementation looks good, however, since multiple sources for duration are allowed in the current logic, implementation must become more complicated. Probably we need more test cases with multiple sources with different duration values.

I often see implicitly defined required_operations (they are mostly measure and delay) in multiple places in the Qiskit codebase. I think Target must provide this information with its class attribute so that developers can easily find the minimum instruction requirements for transpiler. What do you think @mtreinish ?

nkanazawa1989 · 2024-01-05T02:35:31Z

+            if name in qiskit_inst_mapping:
+                inst_name_map[name] = qiskit_inst_mapping[name]
+            else:
+                warnings.warn(


In the original implementation this raises KeyError, and this is sort of breaking API change since user may write try-except code.

Yes, you are right keeping 'KeyError' is wise move here, because user might catch the error, add something in custom_name_mapping or basis_gates etc and run .from_configuration() all over again to get the target.
I thought I would change it to TranspilerError, if you do not like I would put KeyError back.

nkanazawa1989 · 2024-01-05T02:49:36Z

                else:
                    raise TranspilerError(
-                        f"The specified basis gate: {gate} has {gate_obj.num_qubits} "
+                        f"The specified basis gate: {name} has {inst.num_qubits} "


Can you remind me why this doesn't support >2 qubits? I just want to remember why we implemented like this.

I really didn't want to keep it like that, but I am very reluctant to do changes in original code base, so I kept it that way.
This is all I have got:
https://github.com/Qiskit/qiskit/pull/9255/files#r1162884200
and,
#11405 (comment)

Apologies

nkanazawa1989 · 2024-01-05T03:17:22Z

+            prop_name_map["measure"] = {}
+            for qubit_idx in range(num_qubits):
+                qubit_prop = backend_properties.qubit_property(qubit_idx)
+                duration = (


The same logic must be applied here because instruction duration and inst map can be also another source of duration (this design itself is not good though).

I was not sure if the docstrings should be changed or the logic itself should be changed. That's why I request your guidance :)
Applied suggestion :)

I request you to tell a bit more about the flaw in the design :)

nkanazawa1989 · 2024-01-05T03:27:11Z

+                # Map required ops to each operational qubit
+                if prop_name_map[op] is None:
+                    prop_name_map[op] = {
+                        (q,): None for q in range(num_qubits) if q not in faulty_qubits


This is bit fragile because there is no guarantee that op is always single qubit operation.

removed the logic all together,
[humble suggestion]:
when we will add something which is not a single qubit operation in required_operations, then in the same PR we will deal with this at that time.

MozammilQ · 2024-01-05T12:39:00Z

I am done :)

MozammilQ · 2024-01-12T08:17:47Z

I often see implicitly defined required_operations (they are mostly measure and delay) in multiple places in the Qiskit codebase. I think Target must provide this information with its class attribute so that developers can easily find the minimum instruction requirements for transpiler. What do you think @mtreinish ?

Not, sure my vote counts, but
1 Vote in for "adding required_operations as class attribute in Target"

MozammilQ · 2024-01-12T08:18:44Z

@nkanazawa1989 , please see if this is good enough :)

MozammilQ · 2024-03-29T01:25:01Z

@nkanazawa1989 , Please, see if this is good enough :)

MozammilQ · 2024-10-29T22:29:22Z

@1ucian0 , Things have changed in Qiskit, I doubt if the PR is required anymore,
does this PR stands a chance of merging, if yes, what should be the direction of this PR ?

jakelishman · 2026-01-15T01:26:02Z

Closing as stale

MozammilQ and others added 3 commits December 27, 2023 13:37

initial_commit

2a5b5d3

Merge branch 'Qiskit:main' into update_target_from_configuration

6a2f87c

refactor code

4006298

MozammilQ requested a review from a team as a code owner December 27, 2023 17:48

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Dec 27, 2023

MozammilQ added 5 commits December 27, 2023 18:24

refactor code

c9b6ec5

modified tests and added release note

f2e3034

refactor code

3b76ed9

refactor code/tests

65fdea4

refactor code/tests

7c200c2

nkanazawa1989 added the mod: transpiler Issues and PRs related to Transpiler label Jan 5, 2024

to24toro reviewed Jan 5, 2024

View reviewed changes

nkanazawa1989 suggested changes Jan 5, 2024

View reviewed changes

Merge branch 'Qiskit:main' into update_target_from_configuration

a60a5b9

MozammilQ added 10 commits January 5, 2024 12:56

refactor code, altered release note

eb01bd7

refactor code

b49b880

refactor code

567c319

refactor code; added tests

d8f1f60

refactored tests

4156fbe

lint

71d54c6

refactored code

08ce75a

refactored code; added tests

9a12fde

refactor code

6d66a9e

lint

87e8f36

MozammilQ requested review from nkanazawa1989 and to24toro January 12, 2024 08:19

MozammilQ and others added 6 commits January 31, 2024 19:52

Merge branch 'main' into update_target_from_configuration

0abc12a

lint

d5a8e23

Merge branch 'main' into update_target_from_configuration

1c77802

added Generic Backend for tests

d888ea6

fix docs

eaf5974

Merge branch 'main' into update_target_from_configuration

2a56570

1ucian0 mentioned this pull request Apr 10, 2024

introduce default_passmanager to replace generate_preset_pass_manager #12161

Closed

raynelfss mentioned this pull request May 23, 2024

Adding Target and InstructionProperties representations to rust #12292

Merged

3 tasks

Merge branch 'main' into update_target_from_configuration

7734ea0

jakelishman closed this Jan 15, 2026

github-project-automation Bot moved this to Done in Contributor Monitoring Jan 15, 2026

Conversation

MozammilQ commented Dec 27, 2023

Summary

Uh oh!

qiskit-bot commented Dec 27, 2023

Uh oh!

MozammilQ commented Dec 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 8315422102

Details

💛 - Coveralls

Uh oh!

MozammilQ commented Jan 2, 2024

Uh oh!

to24toro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

MozammilQ commented Jan 12, 2024

Uh oh!

MozammilQ commented Jan 12, 2024

Uh oh!

MozammilQ commented Mar 29, 2024

Uh oh!

MozammilQ commented Oct 29, 2024

Uh oh!

jakelishman commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MozammilQ commented Dec 27, 2023 •

edited

Loading

coveralls commented Dec 29, 2023 •

edited

Loading

MozammilQ Jan 12, 2024 •

edited

Loading

MozammilQ commented Jan 5, 2024 •

edited

Loading