Extend the basis gates of BasicSimulator #12186

1ucian0 · 2024-04-16T11:12:42Z

The simulator BasicSimulator is python-based simulator included in qiskit.providers.basic_provider. After the discussion in #10673, Qiskit resolved that every standard gate, up to 3 qubits, can be in BasicSimulator.

The new basis support was extended to: , ccx, ccz, ch, cp, crx, cry, crz, cs, csdg, cswap, csx, cu, cu1, cu3, cx, cy, cz, dcx, delay, ecr, global_phase, h, id, iswap, measure, p, r, rccx, reset, rx, rxx, ry, ryy, rz, rzx, rzz, s, sdg, swap, sx, sxdg, t, tdg, u, u1, u2, u3, unitary, x, xx_minus_yy, xx_plus_yy, y, and z.

qiskit-bot · 2024-04-16T11:12:48Z

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

@Qiskit/terra-core

ElePT

Thanks for the PR @1ucian0, would it make sense to add a few tests to check that the extended support works?

coveralls · 2024-04-19T08:23:44Z

Pull Request Test Coverage Report for Build 8853263480

Details

28 of 28 (100.0%) changed or added relevant lines in 2 files are covered.
5 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.1%) to 89.327%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	5	92.11%

Totals
Change from base Build 8851171785:	-0.1%
Covered Lines:	35980
Relevant Lines:	40279

💛 - Coveralls

qiskit/circuit/library/standard_gates/x.py

1ucian0 · 2024-04-22T01:32:24Z

qiskit/providers/basic_provider/basic_simulator.py

                if operation.name == "unitary":
                    qubits = operation.qubits
                    gate = operation.params[0]
                    self._add_unitary(gate, qubits)
+                elif operation.name in ("id", "u0", "delay"):


I'm not fully sure that delay is the same as id in BasicSimulator

Given that BasicSimulator doesn't understand pulses, I think that the choice is between accepting delay and treating it as an identity or failing if a circuit contains a delay. I would vote for accepting it but raising a user warning to err on the side of caution, given that users often don't understand the capabilities of each simulator.

@jakelishman what do you think? Should be delay be id, or warning, or not accepted?

The basic simulator is noiseless, and in noiseless quantum-state evolution a time-delay is the identity op. I think it's fine / correct to accept delay and do nothing.

ElePT

Thanks @1ucian0 for adding the tests and handling the edge cases. The PR looks good to me, I just have two minor suggestions. One is an inline suggestion about raising a user warning with delay, and the other one would be to include a transpilation step in the run tests, just to make sure that the pass manager runs successfully in case somebody chooses a BasicSimulator as target. I checked out your PR locally and confirmed that it works so this can also be left to a follow-up.

ElePT · 2024-04-23T11:13:40Z

qiskit/providers/basic_provider/basic_simulator.py

                if operation.name == "unitary":
                    qubits = operation.qubits
                    gate = operation.params[0]
                    self._add_unitary(gate, qubits)
+                elif operation.name in ("id", "u0", "delay"):
+                    pass


What about a message like this one?

Suggested change

pass

if operation.name == "delay":

warnings.warn(

"BasicSimulator cannot simulate gate durations, "

"treating delay operation as an identity.",

UserWarning,

stacklevel=2,

)

pass

I think we're fine without a warning; it's completely valid to simulate a delay as an identity if there's no noise in the system.

1ucian0 · 2024-04-23T15:37:55Z

the other one would be to include a transpilation step in the run tests, just to make sure that the pass manager runs successfully in case somebody chooses a BasicSimulator as target.

In e1ad7cb I added the conversion to target. I feel that checking transpilation is checking too much. If the backend can convert to a target and transpile does something wrong with it, it is a different issue.

ElePT

#12186 (comment) testing the target seems fair. I am also not too worried about delay because I think that treating it as id is the closest we can get with a statevector simulator (so in any case it isn't wrong in my opinion), so in the interest of time I am going to approve.

jakelishman · 2024-04-26T10:53:22Z

qiskit/circuit/library/standard_gates/x.py

-            ctrl_state: control state expressed as integer,
-                string (e.g.``'110'``), or ``None``. If ``None``, use all 1s.
+            ctrl_state: control state expressed as integer, string (e.g.``"110"``), or ``None``.
+            If ``None``, use all 1s.


This will have broken the formatting because the continuation line is unindented, and I don't think it'll have actually fixed the problem, which is that there's no space betwen e.g. and the opening ``.

let's try in c35b9cf

jakelishman · 2024-04-26T10:55:32Z

qiskit/providers/basic_provider/basic_provider_tools.py

-
-    if gate == "U":
+    if gate in ("U", "u"):
        gc = gates.UGate
+    elif gate == "u1":
+        gc = gates.U1Gate
+    elif gate == "u2":
+        gc = gates.U2Gate
    elif gate == "u3":
        gc = gates.U3Gate
    elif gate == "h":
        gc = gates.HGate
-    elif gate == "u":
-        gc = gates.UGate
    elif gate == "p":
        gc = gates.PhaseGate


If we're going to add this many new gates, please can we switch to using a dict lookup to retrieve the gate classes?

done in 5cddee7

jakelishman · 2024-04-26T10:56:57Z

qiskit/providers/basic_provider/basic_simulator.py

                if operation.name == "unitary":
                    qubits = operation.qubits
                    gate = operation.params[0]
                    self._add_unitary(gate, qubits)
+                elif operation.name in ("id", "u0", "delay"):
+                    pass


I think we're fine without a warning; it's completely valid to simulate a delay as an identity if there's no noise in the system.

jakelishman · 2024-04-26T10:58:28Z

releasenotes/notes/fixes_10852-e197344c5f44b4f1.yaml

+upgrade_providers:
+  - |
+    The :class:`.BasicSimulator` python-based simulator included in :mod:`qiskit.providers.basic_provider`
+    now includes all the standard gates (:mod:`qiskit.circuit.library .standard_gates`) up to 3 qubits. The new basis support includes:


This is maybe a feature rather than an upgrade? Fwiw, I probably wouldn't bother listing all the gates below - "includes all the standard gates in :mod:`qiskit.circuit.library`" is descriptive enough.

done in e81740e

ElePT

I think that you have implemented all of @jakelishman's comments, so I will approve again and wait to see if he's got any final remark.

* extend the basis gates of BasicSimulator * remove variational sized gates * reno and test * accidentally missing u2 and u1 * three qubits support * support for parameters * support for all the standard gates upto 3 qubits * test * adding rccx * missing gates * test notes * readjust test * check target * tt * dict lookup * reno

extend the basis gates of BasicSimulator

ac92992

1ucian0 requested review from jyu00 and a team as code owners April 16, 2024 11:12

1ucian0 added the Changelog: New Feature Include in the "Added" section of the changelog label Apr 16, 2024

1ucian0 added this to the 1.1.0 milestone Apr 16, 2024

ElePT reviewed Apr 16, 2024

View reviewed changes

1ucian0 added 4 commits April 17, 2024 12:07

Merge branch 'main' of github.com:Qiskit/qiskit-terra into fixes/10852/1

fce3db0

remove variational sized gates

4769932

reno and test

5c29bf6

accidentally missing u2 and u1

5211c68

jakelishman assigned ElePT Apr 18, 2024

1ucian0 added 3 commits April 19, 2024 08:50

three qubits support

5d94a57

Merge branch 'main' of github.com:Qiskit/qiskit-terra into fixes/10852/1

0d2dc4f

support for parameters

21a80c6

1ucian0 added 3 commits April 20, 2024 07:58

support for all the standard gates upto 3 qubits

7b514ce

test

742faa4

adding rccx

89974a1

1ucian0 commented Apr 21, 2024

View reviewed changes

qiskit/circuit/library/standard_gates/x.py Outdated Show resolved Hide resolved

missing gates

de02ac2

1ucian0 commented Apr 22, 2024

View reviewed changes

1ucian0 added 2 commits April 22, 2024 03:33

test notes

bad5565

readjust test

fa28754

ElePT reviewed Apr 23, 2024

View reviewed changes

check target

e1ad7cb

ElePT previously approved these changes Apr 25, 2024

View reviewed changes

jakelishman reviewed Apr 26, 2024

View reviewed changes

Merge branch 'main' of github.com:Qiskit/qiskit-terra into fixes/10852/1

bff2360

tt

c35b9cf

1ucian0 dismissed ElePT’s stale review via c35b9cf April 26, 2024 18:53

1ucian0 added 2 commits April 26, 2024 21:10

dict lookup

5cddee7

reno

e81740e

ElePT approved these changes Apr 29, 2024

View reviewed changes

ElePT added this pull request to the merge queue Apr 30, 2024

Merged via the queue into Qiskit:main with commit 7ec2c56 Apr 30, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend the basis gates of BasicSimulator #12186

Extend the basis gates of BasicSimulator #12186

1ucian0 commented Apr 16, 2024 •

edited

Loading

qiskit-bot commented Apr 16, 2024

ElePT left a comment

coveralls commented Apr 19, 2024 •

edited

Loading

1ucian0 Apr 22, 2024

ElePT Apr 23, 2024

1ucian0 Apr 23, 2024

jakelishman Apr 26, 2024

ElePT left a comment

ElePT Apr 23, 2024

jakelishman Apr 26, 2024

1ucian0 commented Apr 23, 2024

ElePT left a comment

jakelishman Apr 26, 2024

1ucian0 Apr 26, 2024

jakelishman Apr 26, 2024

1ucian0 Apr 26, 2024 •

edited

Loading

jakelishman Apr 26, 2024

jakelishman Apr 26, 2024

1ucian0 Apr 26, 2024

ElePT left a comment

-                    pass
+                    if operation.name == "delay":
+                        warnings.warn(
+                            "BasicSimulator cannot simulate gate durations, "
+                            "treating delay operation as an identity.",
+                            UserWarning,
+                            stacklevel=2,
+                        )
+                    pass

Extend the basis gates of BasicSimulator #12186

Extend the basis gates of BasicSimulator #12186

Conversation

1ucian0 commented Apr 16, 2024 • edited Loading

qiskit-bot commented Apr 16, 2024

ElePT left a comment

Choose a reason for hiding this comment

coveralls commented Apr 19, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8853263480

Details

💛 - Coveralls

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

ElePT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ucian0 commented Apr 23, 2024

ElePT 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

1ucian0 Apr 26, 2024 • edited Loading

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

ElePT left a comment

Choose a reason for hiding this comment

1ucian0 commented Apr 16, 2024 •

edited

Loading

coveralls commented Apr 19, 2024 •

edited

Loading

1ucian0 Apr 26, 2024 •

edited

Loading