Spectroscopy and calibrations integration V3 by eggerdj · Pull Request #88 · qiskit-community/qiskit-experiments

eggerdj · 2021-06-07T15:32:16Z

Added required functionality to update spectroscopy.

Summary

This is a WIP PR to integrate calibration experiments with the Calibrations. Alternatives are in PRs #80 and #79.

Details and comments

In this PR we integrate the experiments and calibrations using an update method. The execution from the user's perspective is

spec = QubitSpectroscopy(3)
spec_data = spec.run()
calibrations.update(spec_data)

* Added required functionality to update spectroscopy.

nkanazawa1989

Thanks Daniel. I think this is nice approach though I see some complexity of mapping of experiment to parameter keys. The main question is who manages the extraction class instances.

Do you think how many experiments need this extraction? I think this is limited to rough amplitude calibration classes. If so maybe we can manage this in Rabi experiment analysis class, rather than introducing new (more general) framework.

Angle cals are also applied to multiple schedules, but this just needs a list of schedules and maybe we can write a list of schedules to update in one of experiment options.

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

nkanazawa1989

I think this PR is in good shape. Few minor comments but these don't block merging.

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

…brations_spec_integration_v3

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

* Changed import order. * Renamed _update to _add_parameter_value.

nkanazawa1989

Perhaps we need to relax the experiment instance check in Amplitude, but for now this looks good to me. When we add different types of calibration and current logic becomes messy, then we can update this in future PR.

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

eggerdj · 2021-06-22T08:56:58Z

Perhaps we need to relax the experiment instance check in Amplitude, but for now this looks good to me. When we add different types of calibration and current logic becomes messy, then we can update this in future PR.

Yes that is why I left it as is. I would rather implemented the other amplitude calibration experiments first and then refactor the inner workings of Amplitude.

wshanks

Here is some minor feedback. I will approve because I don't think any of this should block merging if you want to merge now.

Tests of the updaters might be better placed in a test_update_library module rather than spread out with the experiment tests.
I had thought the updaters would look at analysis result values instead of fit parameters. I had thought the spectroscopy experiment produced an analysis result labeled "frequency" the way the T1 experiment produces a result labeled "T1". What do you think about analysis result values vs. fit parameter keys?
I am not sure about the classmethod usage in the updaters. I think classmethods are a little confusing in general for non-expert Python programmers and also defining classes that you never instantiate is a little confusing. I understand the motivation for doing it this way, but I wonder if someone else wanting to modify an updater class in the future would. Maybe the docstrings should note this usage pattern though I don't know that such a docstring would be noticed. Maybe BaseUpdater.__init__ could raise an exception stating that updaters are not meant to be instantiated?

nkanazawa1989 · 2021-06-22T15:36:03Z

I think these are reasonable suggestions and worth considering.

…/qiskit-experiments into calibrations_spec_integration_v3

…ing. * _add_parameter_value is a classmethod again.

* * Added the update method to the calibrations. * Added required functionality to update spectroscopy. * * Used ParameterValue in update. * Update qiskit_experiments/calibration/calibrations.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * * Updated docstring. * * Added completion times. * * More robust quality check. * * Changed default result index to -1. * * Updated spectroscopy integration. * * Renamed calibration_types to calibration_key_types. * * Removed CalibrationExtraction. * * Removed CalibrationExtraction. * * Black lint * Update qiskit_experiments/calibration/calibration_key_types.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * * Added name for readability. * * Updated docstring. * Update qiskit_experiments/calibration/calibrations.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * * Updated default timestamp to now. * Update qiskit_experiments/calibration/calibrations.py Co-authored-by: Will Shanks <wshaos@posteo.net> * * Added an updater library. * * Made update methodology more permissible. * Reoved the previous update methodology. * * Black * * Moved methodology to class methods. * * Bug fixes, black, and lint. * * Qubits in Rabi. * Update qiskit_experiments/calibration/update_library.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * * Improved docstring. * Changed import order. * Renamed _update to _add_parameter_value. * Update qiskit_experiments/calibration/update_library.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * * Raise on Update instantiation. * * Moved cals update tests to their own library. * * Renamed test. * * Made _add_parameter_value a static method. * * Switched arguments order in update library. * * Added edgecase handling in the calibrations for parameter value adding. * _add_parameter_value is a classmethod again. * * Changed behaviour of get_parameter_value. * * Removed the adding of a microsecond. * * Test docstring fix. Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> Co-authored-by: Will Shanks <wshaos@posteo.net>

eggerdj added 2 commits June 7, 2021 17:25

* Added the update method to the calibrations.

19f8899

* Added required functionality to update spectroscopy.

* Used ParameterValue in update.

a19c821

eggerdj mentioned this pull request Jun 7, 2021

Integrate experiments and calibrations #81

Closed

eggerdj requested a review from nkanazawa1989 June 8, 2021 06:46

nkanazawa1989 reviewed Jun 8, 2021

View reviewed changes

eggerdj and others added 12 commits June 10, 2021 13:43

Update qiskit_experiments/calibration/calibrations.py

71553c7

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

Merge branch 'main' into calibrations_spec_integration_v3

4904e42

* Updated docstring.

9c6ef7f

* Added completion times.

299c959

* More robust quality check.

43a00cf

* Changed default result index to -1.

383fc08

* Updated spectroscopy integration.

61c0920

* Renamed calibration_types to calibration_key_types.

c1e29db

* Removed CalibrationExtraction.

4f2f40f

* Removed CalibrationExtraction.

c7a4b71

* Black lint

f276c7b

Merge branch 'main' into calibrations_spec_integration_v3

991f8ea

eggerdj added this to the Release 1 milestone Jun 14, 2021

nkanazawa1989 reviewed Jun 15, 2021

View reviewed changes

eggerdj and others added 6 commits June 15, 2021 09:26

Update qiskit_experiments/calibration/calibration_key_types.py

9a76125

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

Merge branch 'main' into calibrations_spec_integration_v3

f6f2d83

* Added name for readability.

3fea5a4

* Updated docstring.

722ba59

Update qiskit_experiments/calibration/calibrations.py

4e31847

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

* Updated default timestamp to now.

ae4a7c9

eggerdj changed the title ~~[WIP] Spectroscopy and calibrations integration V3~~ Spectroscopy and calibrations integration V3 Jun 15, 2021

Merge branch 'main' into calibrations_spec_integration_v3

f032b5d

nkanazawa1989 approved these changes Jun 16, 2021

View reviewed changes

Merge branch 'main' into calibrations_spec_integration_v3

0d8fc79

wshanks reviewed Jun 16, 2021

View reviewed changes

Comment thread qiskit_experiments/calibration/calibrations.py Outdated

eggerdj added 2 commits June 21, 2021 18:02

* Qubits in Rabi.

864f3af

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

744bb17

…brations_spec_integration_v3

nkanazawa1989 suggested changes Jun 22, 2021

View reviewed changes

Comment thread qiskit_experiments/calibration/update_library.py Outdated

Comment thread qiskit_experiments/calibration/update_library.py Outdated

eggerdj and others added 2 commits June 22, 2021 08:43

Update qiskit_experiments/calibration/update_library.py

b818297

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

* Improved docstring.

8e65eef

* Changed import order. * Renamed _update to _add_parameter_value.

nkanazawa1989 approved these changes Jun 22, 2021

View reviewed changes

nkanazawa1989 reviewed Jun 22, 2021

View reviewed changes

Comment thread qiskit_experiments/calibration/update_library.py Outdated

Update qiskit_experiments/calibration/update_library.py

23b3a04

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

wshanks approved these changes Jun 22, 2021

View reviewed changes

eggerdj added 3 commits June 22, 2021 17:42

* Raise on Update instantiation.

d4c792d

Merge branch 'calibrations_spec_integration_v3' of github.com:eggerdj…

fac68ec

…/qiskit-experiments into calibrations_spec_integration_v3

* Moved cals update tests to their own library.

c2b4109

wshanks approved these changes Jun 22, 2021

View reviewed changes

eggerdj added 2 commits June 22, 2021 19:16

* Renamed test.

90b5617

* Made _add_parameter_value a static method.

762ded0

boschmitt reviewed Jun 23, 2021

View reviewed changes

Comment thread qiskit_experiments/calibration/update_library.py

eggerdj and others added 4 commits June 23, 2021 12:15

* Switched arguments order in update library.

ce9b9fc

* Added edgecase handling in the calibrations for parameter value add…

cac9706

…ing. * _add_parameter_value is a classmethod again.

Merge branch 'main' into calibrations_spec_integration_v3

e1e5a72

* Changed behaviour of get_parameter_value.

4bb5861

nkanazawa1989 approved these changes Jun 23, 2021

View reviewed changes

eggerdj added 2 commits June 23, 2021 18:59

* Removed the adding of a microsecond.

1262246

* Test docstring fix.

206baa1

eggerdj merged commit c2aa037 into qiskit-community:main Jun 23, 2021

eggerdj deleted the calibrations_spec_integration_v3 branch June 23, 2021 17:34

This was referenced Jun 23, 2021

[WIP] Spectroscopy and calibrations integration V1 #80

Closed

[WIP] Spectroscopy and calibrations integration V2 #79

Closed

Conversation

eggerdj commented Jun 7, 2021

Summary

Details and comments

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eggerdj commented Jun 22, 2021

Uh oh!

wshanks left a comment

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 commented Jun 22, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants