Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved measurements timings #129

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

maij
Copy link
Collaborator

@maij maij commented May 5, 2021

Fixes a bug that prevented certain timings from being recorded correctly.

Measurements now record the time between each measured value (stored as pre_msmt_time) which can be useful to pinpoint wasted time, especially with significant setup or analysis between measurements.

N.B. the time to save traces is handled by the layout, which is a layer above the qcodes measurement context, so we cannot easily include that as a separate value. The time taken to acquire a parameter (i.e. to perform layout.acquisition + analysis) includes the time to save traces.

Example code and timings:

dummy = Parameter('dummy', get_cmd=lambda: 1)
dummy2 = Parameter(get_cmd=lambda:2)

with Measurement('test') as msmt:
    for _ in Sweep(range(10)):
        msmt.measure(dummy)
        msmt.measure(dummy)
        sleep(0.333)
        msmt.measure(123, 'test_other_name')
        sleep(np.random.rand())
        msmt.measure(dummy2)
        msmt.measure({'hello': 4123, 'world':13}, 'dict')
        sleep(0.21)

As expected, the measurements with sleep commands before them have larger pre_msmt_times than the others.
This seems to work for all current measurables.

{ 'dataset': { 'finalize': '30.9+-0 ms',
               'save_metadata': '12.3+-18.7 ms',
               'store': '191+-303 us'},
  'measurement': { (0, 0): { 'dummy': '6.46+-18.6 ms',
                             'pre_msmt_time': '191+-59.9 ms',
                             'total': '197+-41.4 ms'},
                   (0, 1): { 'dummy': '620+-1.28e+03 us',
                             'pre_msmt_time': '11.6+-7.25 us',
                             'total': '706+-1.26e+03 us'},
                   (0, 2): { 'pre_msmt_time': '333+-0.112 ms',
                             'test_other_name': '896+-1.77e+03 us',
                             'total': '335+-1.69 ms'},
                   (0, 3): { 'None': '1.44+-2.33 ms',
                             'pre_msmt_time': '542+-325 ms',
                             'total': '543+-324 ms'},
                   (0, 4): { 'dict': '2.09+-4.26 ms',
                             'pre_msmt_time': '12.9+-3.42 us',
                             'total': '55.2+-11.6 us'},
                   (0, 4, 0): { 'hello': '915+-2.19e+03 us',
                                'pre_msmt_time': '420+-1.35e+03 us',
                                'total': '1.13+-2.2 ms'},
                   (0, 4, 1): { 'pre_msmt_time': '42.1+-12.3 us',
                                'total': '941+-2.06e+03 us',
                                'world': '836+-2.05e+03 us'}}}

maij added 4 commits May 5, 2021 22:08
This doesn't work when its decorated as a context manager
Currently not robust, maybe we should determine the name up-front from all the optional callables, else copy this code snippet through all functions.
Duplicating these lines of code is a lot easier than timing the measure function due to the difficulty of determining the measured `name`
@maij maij requested a review from Ifefu October 10, 2021 23:08
@maij maij requested a review from HollySte January 20, 2022 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant