Remove units from experiment options #516

yaelbh · 2021-11-09T14:12:11Z

Users will not be able to determine the units of their input parameters. When it comes to time units, pulse widths will be in dt, and delays as in T1 and T2 will be in seconds.

nkanazawa1989

Thanks @yaelbh for taking a look at the unit issue. I think there are several other experiments that take frequency val with arbitrary unit. Probably we need an internal rule to define how we pass values to fitter, either dt or SI. If fitter assumes values in SI the xval conversion logic should exist on experiment side.

nkanazawa1989 · 2021-11-09T14:55:06Z

qiskit_experiments/library/characterization/cr_hamiltonian.py

                        "experiment_type": self.experiment_type,
                        "qubits": self.physical_qubits,
-                        "xval": prefactor * flat_top_width * dt_factor,  # in units of sec
+                        "xval": flat_top_width


this conversion is necessary because fitter assumes xval in sec, i.e. it need to compute Hamiltonian coefficients in Hz (or you can covert dt in sec in the fitter since it still has access to backend)

yaelbh · 2021-11-10T08:05:23Z

To do:

Update the characterization experiments: CR Hamitonian, Ramsey XY, T1, T2Ramsey.
Fix their tests.
Fix their tutorials.
Do the same for calibration units that are different from Hz (like kHz and MHz) ?
Release notes.

nkanazawa1989 · 2021-11-11T14:07:04Z

qiskit_experiments/library/characterization/qubit_spectroscopy.py

        options.normalization = True
        options.xlabel = "Frequency"
        options.ylabel = "Signal (arb. units)"
-        options.xval_unit = "Hz"


this xval_unit is necessary because this represents an axis label of output figure.

yaelbh · 2021-11-16T11:06:03Z

I'd like not to update the cloud tutorial. It's not updated to the latest main in the first place, it involves cuts of screenshots from the web, and is about to be refactored anyway.

nkanazawa1989

Thanks @yaelbh this is really good. Just one request for update of handling of delay to be more precise. You can also remove this code
https://github.com/Qiskit/qiskit-experiments/blob/482a83cede9a22fd96bfa176446014d48e57ea61/qiskit_experiments/library/characterization/t1.py#L108-L118
and
https://github.com/Qiskit/qiskit-experiments/blob/482a83cede9a22fd96bfa176446014d48e57ea61/qiskit_experiments/library/characterization/t1.py#L64
because this exist because of init guess unit conversion that is removed with this PR. Then you can revert the patch code introduced in #529

qiskit_experiments/library/calibration/rough_frequency.py

releasenotes/notes/remove-units-78db311686213a58.yaml

nkanazawa1989 · 2021-11-20T09:39:22Z

qiskit_experiments/library/characterization/t1.py

-
        circuits = []
-        for delay in prefactor * np.asarray(self.experiment_options.delays, dtype=float):
+        for delay in np.asarray(self.experiment_options.delays, dtype=float):


Can we explicitly convert delay into dt here? Because delay in SI is converted into dt in the transpiler and rounded to integer. So actual delay value can differ from x value in analysis by this rounded value. This effect becomes visible when T1 value is very short (really edge case though).

The code is something like

delay_dt = int(np.round(delay / dt_factor)) ... circ.delay(delay_dt, 0, "dt") ... "xval": delay_dt * dt_factor,

same for T2

Not sure. On the one hand, I understand your reason. On the other hand:

I was hoping to completely get rid of anything that has units and factors in it.

Users may not understand why the delays they entered have shifted.

I agree mismatch due to rounding error is negligible and probably this is overkill. In our device, the error of actual delay and fit xval is less than 0.22 ns which is way smaller than T1 decay of fit.

I think the values should be shifted, but I also agree that for current IBM devices this concern is negligible for T1. However, there are some experiments where timing is important and I think we should try to handle time values consistently across experiments. So I think it would be best to store time values as the real values when we are scanning time and rounding is involved. Maybe a helper function for translating the time values could help promote handling time values consistently?

Done, but in way that's a bit different from the one suggested here, please check

It looks okay to me, but @nkanazawa1989 should check it. I don't know what one can rely on when doing a transpilation. Could a delay gate ever be split into multiple delays or could another delay gate ever get inserted in the beginning of a circuit?

I think this is too hacky and has several edge cases. For example, you only assumes single delay in the program but this is not true because measure align pass will add extra delay before measurement to align acquire instruction. In addition, T2Ramsey needs delay to insert artificial rotation and the rotation angle should be computed with correct delay amount. Of course calling transpile for computing xval is too much extra computation overhead. Why just cannot

delays_dt = np.round(options.delays / self.backend.configuration().dt).astype(int) for delay_dt in delays_dt: delay_sec_rounded = delay_dt * self.backend.configuration().dt # used for T2Ramsey and xval qc.delay(delay_dt, 0, "dt")

Indeed this is what is done during transpilation.

If you are still skeptical, you can call this function from terra. This is what exactly called when you transpile delays with value in sec (but warnings are super annoying).
https://github.com/Qiskit/qiskit-terra/blob/f6ab3a98ca2c8c33842e1adbf5da6fbde78dc268/qiskit/circuit/duration.py#L23-L42

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

yaelbh · 2021-11-21T11:18:32Z

Reporting that I was able to successfully run on a device and save to the database.

…to dt

qiskit_experiments/library/characterization/t1.py

nkanazawa1989 · 2021-11-25T14:32:15Z

qiskit_experiments/library/characterization/t2ramsey.py

+
        circuits = []
        for delay in self.experiment_options.delays:
            rotation_angle = 2 * np.pi * self.experiment_options.osc_freq * delay


You also need to update delay here (will be delay_dt * dt_factor)

nkanazawa1989 · 2021-11-30T03:31:35Z

qiskit_experiments/library/characterization/t2ramsey.py

-            delay = np.round(delay, decimals=10)
+        for delay in self.experiment_options.delays:
+            if dt_unit:
+                real_delay_in_sec = delay_dt * dt_factor


seems like delay_dt is used before assignment. how is this test passed? seems like we are testing only on the backend without dt.

Yes, this is how we test, but I always run on a device and look at the results, so I can't explain.

Actually I can explain: I've had other, unrelated problems with T2Ramsey-on-device lately, in the main branch, I think I opened issues. So I didn't run.

nkanazawa1989 · 2021-11-30T03:34:15Z

qiskit_experiments/library/characterization/ramsey_xy.py

        ram_x = self._pre_circuit()
        ram_x.sx(0)
-        ram_x.delay(p_delay, 0, self.experiment_options.unit)
+        ram_x.delay(p_delay, 0, "s")


could you please also update this in the same way with t2ramsey?

nkanazawa1989

Thanks @yaelbh I'll approve with update of RamseyXY.

nkanazawa1989 · 2021-12-08T08:43:05Z

This is API change and should be merged in 0.2. Any update? @yaelbh ? I found that we should set delay in units "dt" rather than "s" for T1, T2Ramsey, RamseyXY to avoid annoying user warning.

Could you please update the logic?

yaelbh · 2021-12-09T06:23:12Z

@nkanazawa1989 I haven't tested on device yet (I'm going to do it now) but essentially it's ready. In ramsey xy I had to work with two parameters because Parameter doesn't support round and int operations (working only with one parameter, there was an error that delay unit in dt must be integer).

nkanazawa1989

Thanks Yael. Code looks good to me. Probably we need to implement some logic to allow typecast of Parameter in future (I had same problem when I write CR ham tomo).

* dt units in cr_hamiltonian * updated characterization experiments * t1 tests * t2ramsey test * cr hamiltonian tests * Ramsey XY tests * black * lint * fixed ramsey xy test * Hz * black * lint * updated t1 tutorial * lint * fixed tests * t1 tutorial again (nicer execution count) * updated t2 ramsey tutorial * more update to t2ramsey tutorial * fixed analysis option in qubit spectro * release notes * Update qiskit_experiments/library/calibration/rough_frequency.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update releasenotes/notes/remove-units-78db311686213a58.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * update xval to the true value following transpilation and conversion to dt * round dts * fixed rotation angle in t2ramsey * bug fix in t2ramsey * resolved conflict * ramsey_xy * transpile options for ramsey xy Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

dt units in cr_hamiltonian

f3abb34

nkanazawa1989 reviewed Nov 9, 2021

View reviewed changes

updated characterization experiments

e41b979

yaelbh added 10 commits November 10, 2021 11:43

t1 tests

0230f39

t2ramsey test

7188ab4

cr hamiltonian tests

96f7722

Ramsey XY tests

94b706e

black

d55b98d

lint

8b49959

fixed ramsey xy test

c492a17

Hz

22aee48

black

3bd7c4a

lint

0639f04

nkanazawa1989 reviewed Nov 11, 2021

View reviewed changes

yaelbh added 9 commits November 16, 2021 11:40

updated t1 tutorial

86bc3fc

Merge branch 'main' into units

78d7817

lint

aae1fa1

fixed tests

38b2188

t1 tutorial again (nicer execution count)

91596ff

updated t2 ramsey tutorial

224e7f0

more update to t2ramsey tutorial

c48acaa

fixed analysis option in qubit spectro

66a7373

release notes

97a3881

yaelbh changed the title ~~[WIP] Remove units from experiment options~~ Remove units from experiment options Nov 16, 2021

yaelbh added 2 commits November 17, 2021 06:59

Merge branch 'main' into units

326a962

Merge branch 'main' into units

ac1f28b

nkanazawa1989 requested changes Nov 20, 2021

View reviewed changes

Merge branch 'main' into units

7ed9ab9

yaelbh and others added 2 commits November 21, 2021 08:40

Update qiskit_experiments/library/calibration/rough_frequency.py

cc5dd35

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

Update releasenotes/notes/remove-units-78db311686213a58.yaml

c337c64

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

yaelbh added 4 commits November 21, 2021 14:27

Merge branch 'main' into units

d668c81

update xval to the true value following transpilation and conversion …

646ce76

…to dt

round dts

497fef7

Merge branch 'main' into units

9e0e250

yaelbh requested a review from nkanazawa1989 November 25, 2021 12:10

nkanazawa1989 requested changes Nov 25, 2021

View reviewed changes

fixed rotation angle in t2ramsey

e470850

yaelbh requested a review from nkanazawa1989 November 28, 2021 07:44

yaelbh mentioned this pull request Nov 29, 2021

Revisit units in API #456

Closed

yaelbh linked an issue Nov 29, 2021 that may be closed by this pull request

Revisit units in API #456

Closed

nkanazawa1989 requested changes Nov 30, 2021

View reviewed changes

nkanazawa1989 reviewed Nov 30, 2021

View reviewed changes

bug fix in t2ramsey

b751c0e

nkanazawa1989 reviewed Dec 2, 2021

View reviewed changes

nkanazawa1989 added this to the Release 0.2 milestone Dec 8, 2021

Merge branch 'main' into units

f3f472c

wshanks mentioned this pull request Dec 8, 2021

Errors when loading T1 and T2Ramsey experiments #542

Closed

yaelbh added 2 commits December 9, 2021 07:30

resolved conflict

6668412

ramsey_xy

28bc939

transpile options for ramsey xy

ff2684e

nkanazawa1989 approved these changes Dec 9, 2021

View reviewed changes

Merge branch 'main' into units

519abd6

yaelbh merged commit a6f1f67 into qiskit-community:main Dec 9, 2021

yaelbh deleted the units branch December 9, 2021 09:29

Remove units from experiment options #516

Remove units from experiment options #516

Uh oh!

Conversation

yaelbh commented Nov 9, 2021

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaelbh commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaelbh commented Nov 16, 2021

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 Nov 24, 2021 • 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!

yaelbh commented Nov 21, 2021

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 commented Dec 8, 2021

Uh oh!

yaelbh commented Dec 9, 2021

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yaelbh commented Nov 10, 2021 •

edited

Loading

nkanazawa1989 Nov 24, 2021 •

edited

Loading