Rework of Stark module and workflow #1236

nkanazawa1989 · 2023-07-26T05:27:55Z

Summary

This PR moves all Stark experiments to new module qiskit_experiments.library.driven_freq_tuning to define a module-wise utility file. This util file contains the StarkCoefficients dataclass to combine all seven coefficients from third-order polynomial fit characterizing the Stark shift. This object is shared among all experiments and analyses in new module.

In addition to this, StarkP1Spectroscopy allows users to scan xval in units of either amplitude or frequency (previously only amplitude was allowed). These two domains are mutually convertible with the StarkCoefficients object. The domain conversion functions are also included in the util file.

Details and comments

Experiment option names are updated to be more general, namely amp -> xval and new option xval_type is added. xval_type is either amplitude or frequency. Experimentalist can directly specify the target Stark shift by

exp = StarkP1Spectroscopy((0,), backend=backend)

freqs = np.linspace(-70e6, 70e6, 31)
exp.set_experiment_options(
    xvals=freqs,
    xval_type="frequency",
)

Note that this requires pre-calibration of Stark shift coefficients with StarkRamseyXYAmpScan experiment to convert specified frequencies into tone amplitudes, and one must save the calibration results in the experiment service. If the service is not available, one can also directly provide these coefficients instead of providing a service through the backend.

exp.set_experiment_options(
    xvals=test_freqs,
    xval_type="frequency",
    stark_coefficients=StarkCoefficients(...),
)

When the coefficients are already calibrated, one can estimate the maximum Stark shift available within the power budget.

min_freq, max_freq = util.find_min_max_frequency(-0.9, 0.9, coeffs)

wshanks

This looks good. Besides some wording suggestions, I wanted to know what you think about having a more standardized location for the frequency shift polynomial coefficients. Other than that I think this is ready to merge.

qiskit_experiments/library/characterization/analysis/t1_analysis.py

qiskit_experiments/library/characterization/t1.py

test/library/characterization/test_stark_p1_spect.py

qiskit_experiments/library/characterization/t1.py

Co-authored-by: Will Shanks <[email protected]>

…ure/stark_p1_frequency_scan

- reorganize; move all experiments and analyses to own library.driven_freq_tuning - add util; define StarkCoefficients dataclasses and util functions - separation; delegated the role of coefficient manipulation from the analysis class to util functions

nkanazawa1989 · 2024-01-17T13:57:38Z

@wshanks I did major rework of this branch in da444b6 and updated the PR title to align with what it is doing. This is based on what you suggested in the comment, namely, creating new module driven_freq_tuning to combine all stark experiments. This allows us to define module-wide util file containing a dataclass StarkCoefficients, which is shared among all experiments and analyses.

wshanks

This is great. I have one practical issue with retrieving the coefficients from the live service (not sure if we could test that case better here?) and then some questions/suggestions.

qiskit_experiments/library/driven_freq_tuning/coefficient_utils.py

wshanks · 2024-01-22T16:49:10Z

qiskit_experiments/library/driven_freq_tuning/coefficient_utils.py

+            valid_amp = float(valid_amps[before_inflection].real)
+        else:
+            valid_amp = float(valid_amps[0].real)
+        amplitudes[idx] = min(valid_amp, 1.0)


Couldn't you avoid the min(..., 1.0) part if you did not allow the amp to be 10 * np.finfo(float).eps more than 1.0 above?

I coded like you mentioned in the beginning, but found that some test case (round trip case for amplitude input = 1.0) fails due to numerical precision problem. This is the trick to allow full dynamic range more safely.

Okay, I see. So you do a sweep up to amp=1.0 and get a set of coefficients. Then you try to sweep up to the max frequency and get an error because rounding causes the max frequency to correspond to an amp slightly above 1 and be filtered out above.

Maybe a comment would be good: "This clipping is okay because values well above 1.0 were filtered out above". I would consider move this operation just below the filtering so it stays close.

By the way, should it be: np.sign(valid_amp) * min(abs(valid_amp), 1.0)?

wshanks · 2024-01-22T21:08:49Z

qiskit_experiments/library/driven_freq_tuning/coefficient_utils.py

+        retrieved = service.analysis_results(
+            device_components=[f"Q{qubit}"],
+            result_type="stark_coefficients",
+            backend_name=backend_name,
+            sort_by=["creation_datetime:desc"],
+        )


Something is off here for me. IBMExperimentService does not know about the StarkCoefficients class so it should not be able to return an instance of it from analysis_results() here like the type hint suggests. I tried adding json_decoder=ExperimentDecoder, but that does not completely solve the issue. When I do that, I get a retrieved[0].result_data like:

{'value': '(StarkCoefficients)', '_chisq': 8.026329774009517, '_extra': {'run_time': None, 'experiment': 'StarkRamseyXYAmpScan'}, '_value': StarkCoefficients(pos_coef_o1=5748041.745393166, pos_coef_o2=17930671.766640767, pos_coef_o3=7365555.846512856, neg_coef_o1=-581564.1840185896, neg_coef_o2=-27118377.31311793, neg_coef_o3=3073801.6774436794, offset=-658782.8378606051), '_source': {'class': 'qiskit_experiments.framework.analysis_result.AnalysisResult', 'data_version': 1, 'qiskit_version': {'qiskit': '0.45.1', 'qiskit-experiments': '0.6.0'}}}

So it does have the StarkCoefficients instance in _value but it is a dict and 'value' is just a string (StarkCoefficients).

If I don't set the decoder I get

{'value': '(StarkCoefficients)', '_chisq': 8.026329774009517, '_extra': {'run_time': None, 'experiment': 'StarkRamseyXYAmpScan'}, '_value': {'__type__': 'object', '__value__': {'class': {'__type__': 'type', '__value__': {'name': 'StarkCoefficients', 'module': 'qiskit_experiments.library.driven_freq_tuning.coefficient_utils', 'version': '0.6.0.dev0+da444b6'}}, 'version': '0.6.0.dev0+da444b6', 'settings': {'offset': -658782.8378606051, 'neg_coef_o1': -581564.1840185896, 'neg_coef_o2': -27118377.31311793, 'neg_coef_o3': 3073801.6774436794, 'pos_coef_o1': 5748041.745393166, 'pos_coef_o2': 17930671.766640767, 'pos_coef_o3': 7365555.846512856}}}, '_source': {'class': 'qiskit_experiments.framework.analysis_result.AnalysisResult', 'data_version': 1, 'qiskit_version': {'qiskit': '0.45.1', 'qiskit-experiments': '0.6.0'}}}

Oh thanks for catching this. I was thinking the fake service reproduces the behavior of the IBM service, but details are different in many places. For now I fixed the problem in 7521150 and added minimum example code I tested with a real server in3e9a576 as a tutorial.

test/library/driven_freq_tuning/test_stark_p1_spect.py

wshanks · 2024-01-22T21:37:35Z

test/library/driven_freq_tuning/test_stark_p1_spect.py

-            ),
-        )
-        np.testing.assert_array_almost_equal(test_xvals, ref_xvals)
+        analysis.run(exp_data, replace_results=True).block_for_results()


It doesn't matter, but ExperimentData.analysis_results() blocks by default so you don't need to block here. At one point, I started going through the tests adding block_for_results before realizing this because I was worried about race conditions.

wshanks · 2024-01-22T21:53:20Z

qiskit_experiments/library/driven_freq_tuning/coefficient_utils.py

+
+
+@dataclass
+class StarkCoefficients:


I like how this class cleans up the experiment classes. All the individual functions in this module either take StarkCoefficients as an argument or create a StarkCoefficients instance. Maybe they would be better as methods and classmethod alternate constructors on the class instead of separate functions? Then you could call this file just coefficients.py. Whenever a file has util in the name, it tends to attract unrelated stuff.

This suggestion makes perfect sense. I did some refactoring in 7675cbb. Note that I decided to keep two functions for data retrieval separated to decouple the service API from the local object. I think this structure should be fine.

wshanks · 2024-01-22T21:55:29Z

qiskit_experiments/library/driven_freq_tuning/analyses.py

+)
+
+
+class StarkRamseyXYAmpScanAnalysis(curve.CurveAnalysis):


Any reason to put the two classes together here when we usually use separate files? The corresponding experiments use separate files for example. If we want to group classes together more, I think it would make more sense to group the analysis classes with the experiments (StarkRamseyXYAmpScanAnalysis in ramsey_amp_scan.py for example).

No, I just hesitate to have lengthly file name *_analysis.py. But split the file into two according to our convention.

Co-authored-by: Will Shanks <[email protected]>

- Convert util functions to class methods. Change file name to restrict the context. - Split analysis file

wshanks

Some minor comments, look good to me!

qiskit_experiments/library/driven_freq_tuning/__init__.py

wshanks · 2024-01-29T21:05:05Z

qiskit_experiments/library/driven_freq_tuning/coefficient.py

+        RuntimeError: When stark_coefficients entry doesn't exist in the service.
+    """
+    try:
+        if isinstance(qubit, (list, tuple)) and len(qubit) == 1:


Why is this needed, rather than just accepting an int? The type hint should be probably be int | list[int] | tuple[int]. Should it error if the length is not 1? Just fail to find Q[0, 1]?

Good point. Indeed this input violates the type hint. The type cast is removed in 8ba6fe4. I'm used to writing a qubit index with single element tuple, so this was mainly to cope with my habit.

docs/manuals/characterization/stark_experiment.rst

wshanks · 2024-01-29T21:36:51Z

qiskit_experiments/library/driven_freq_tuning/coefficient.py

+from qiskit_experiments.framework.experiment_data import ExperimentData
+
+
+class StarkCoefficients:


Just curious, why did you switch away from a dataclass?

When is_dataclass(my_object) == True I think the object is a simple data container, however, with the refactoring the StarkCoefficients gained several methods that perform more than data formatting. To avoid implementing something that doesn't compatible with dataclass, I decided to convert this into a subclass of bare python object.

I see. That is reasonable. I think there are different perceptions of dataclass in the Python community. Some see it as a simple data container like you mention. Others see it as a helper that removes the boilerplate of writing a self.x = x init method and writing repr and equality methods from otherwise normal classes.

wshanks · 2024-01-29T21:41:24Z

qiskit_experiments/library/driven_freq_tuning/coefficient.py

+    try:
+        if isinstance(qubit, (list, tuple)) and len(qubit) == 1:
+            qubit = qubit[0]
+        retrieved = service.analysis_results(


Should this put limit=1? I noticed that the qiskit-ibm-experiment has a default limit of 10 because when I first tried to test the new code I kept getting errors about coefficient_util not existing. I realized that this was from results from my previous testing still being in the most recent 10 results that were retrieved.

Thanks for finding this. Likely this is an edge case but worth adding the limit. Done in aaee397

Co-authored-by: Will Shanks <[email protected]>

nkanazawa1989 · 2024-01-30T08:29:51Z

Thanks @wshanks for careful review and testing. I think now the code is ready.

wshanks

Looks good!

wshanks · 2024-01-30T17:46:51Z

qiskit_experiments/library/driven_freq_tuning/coefficients.py

+            backend_name=backend_name,
+            sort_by=["creation_datetime:desc"],
+            json_decoder=ExperimentDecoder,
+            # Returns the latest value only. IBM service returns 10 entries by default.


I don't think this comment is necessary. We only want one value so limiting to 1 is reasonable. It should be more efficient, though probably negligibly. The old data that could not be deserialized was just what made me realize in testing that we were retrieving more than one result here.

yes, 1 is reasonable mainly for efficiency but I think it's worth having some note about edge case (so that someone save time for debug)

nkanazawa1989 added 5 commits July 24, 2023 16:00

Add frequency scan option

6ce293f

Add function to calculate min, max frequency value

498f8a6

Improve freq->amp conversion for more robustness

ac22b8e

Update interface of estimate_minmax_frequencies

effe196

Fix lint error and missing doc for experiment options

fda5774

nkanazawa1989 added this to the Release 0.6 milestone Aug 8, 2023

Merge branch 'main' into feature/stark_p1_frequency_scan

98c7e0d

wshanks reviewed Oct 31, 2023

View reviewed changes

nkanazawa1989 force-pushed the feature/stark_p1_frequency_scan branch from 4c2f37c to 98c7e0d Compare January 10, 2024 02:07

Wording suggestions

6e2f441

Co-authored-by: Will Shanks <[email protected]>

nkanazawa1989 changed the title ~~Add frequency scan option to StarkP1Spectroscopy experiment~~ Rework of Stark module and workflow Jan 17, 2024

nkanazawa1989 force-pushed the feature/stark_p1_frequency_scan branch 2 times, most recently from 092a874 to 6e2f441 Compare January 17, 2024 13:52

nkanazawa1989 added 2 commits January 17, 2024 22:52

Merge branch 'main' of github.com:Qiskit/qiskit-experiments into feat…

c759f0a

…ure/stark_p1_frequency_scan

Rework

da444b6

- reorganize; move all experiments and analyses to own library.driven_freq_tuning - add util; define StarkCoefficients dataclasses and util functions - separation; delegated the role of coefficient manipulation from the analysis class to util functions

wshanks reviewed Jan 22, 2024

View reviewed changes

nkanazawa1989 and others added 2 commits January 29, 2024 11:29

Wording

353f377

Co-authored-by: Will Shanks <[email protected]>

Code structure change

7675cbb

- Convert util functions to class methods. Change file name to restrict the context. - Split analysis file

nkanazawa1989 force-pushed the feature/stark_p1_frequency_scan branch from 98f3f34 to 12ee094 Compare January 29, 2024 15:55

nkanazawa1989 added 2 commits January 30, 2024 01:02

Fix retrieve logic

7521150

Update tutorial to show example code

3e9a576

nkanazawa1989 force-pushed the feature/stark_p1_frequency_scan branch from 12ee094 to 3e9a576 Compare January 29, 2024 16:02

wshanks reviewed Jan 29, 2024

View reviewed changes

nkanazawa1989 and others added 4 commits January 30, 2024 17:05

Update file name

6ba5a26

Wording

5cf46a0

Co-authored-by: Will Shanks <[email protected]>

Set limit to number of retrieved entry

aaee397

Remove type cast

8ba6fe4

wshanks approved these changes Jan 30, 2024

View reviewed changes

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

Merged via the queue into qiskit-community:main with commit 46e7eec Jan 31, 2024
11 checks passed

nkanazawa1989 mentioned this pull request Jan 31, 2024

Cleanup dataframes #1360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework of Stark module and workflow #1236

Rework of Stark module and workflow #1236

nkanazawa1989 commented Jul 26, 2023 •

edited

Loading

wshanks left a comment

nkanazawa1989 commented Jan 17, 2024

wshanks left a comment

wshanks Jan 22, 2024

nkanazawa1989 Jan 29, 2024

wshanks Jan 29, 2024

wshanks Jan 22, 2024 •

edited

Loading

nkanazawa1989 Jan 29, 2024 •

edited

Loading

nkanazawa1989 Jan 29, 2024

wshanks Jan 22, 2024

wshanks Jan 22, 2024

nkanazawa1989 Jan 29, 2024

wshanks Jan 22, 2024

nkanazawa1989 Jan 29, 2024

wshanks left a comment

wshanks Jan 29, 2024

nkanazawa1989 Jan 30, 2024

wshanks Jan 29, 2024

nkanazawa1989 Jan 30, 2024

wshanks Jan 30, 2024

wshanks Jan 29, 2024

nkanazawa1989 Jan 30, 2024

nkanazawa1989 commented Jan 30, 2024

wshanks left a comment

wshanks Jan 30, 2024

nkanazawa1989 Jan 31, 2024

		from qiskit_experiments.framework.experiment_data import ExperimentData


		class StarkCoefficients:

Rework of Stark module and workflow #1236

Rework of Stark module and workflow #1236

Conversation

nkanazawa1989 commented Jul 26, 2023 • edited Loading

Summary

Details and comments

wshanks left a comment

Choose a reason for hiding this comment

nkanazawa1989 commented Jan 17, 2024

wshanks 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

wshanks Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

nkanazawa1989 Jan 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wshanks 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

nkanazawa1989 commented Jan 30, 2024

wshanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 commented Jul 26, 2023 •

edited

Loading

wshanks Jan 22, 2024 •

edited

Loading

nkanazawa1989 Jan 29, 2024 •

edited

Loading