Dataset support multiparameter #1249

jenshnielsen · 2018-08-27T18:59:16Z

Fixes parts of #1200

Add support to Measurement to register and add results from multiparamters.
A good deal of refactoring of the logic of add_result and register_parameter to make this possible
Some new convenience functions for setpoint names
The setpoint_full_names functions etc should really have individual tests.
Examples of this would be good

and make it slightly more readable

By ensuring that writeperid is always a float

codecov · 2018-08-28T08:37:16Z

Codecov Report

Merging #1249 into master will increase coverage by 0.15%.
The diff coverage is 92.39%.

@@            Coverage Diff             @@
##           master    #1249      +/-   ##
==========================================
+ Coverage   70.49%   70.64%   +0.15%     
==========================================
  Files          74       74              
  Lines        8094     8160      +66     
==========================================
+ Hits         5706     5765      +59     
- Misses       2388     2395       +7

jenshnielsen · 2018-08-28T08:38:25Z

qcodes/tests/dataset/test_measurement_context_manager.py

@@ -29,7 +30,7 @@ def empty_temp_db():
    # create a temp database for testing
    with tempfile.TemporaryDirectory() as tmpdirname:
        qc.config["core"]["db_location"] = os.path.join(tmpdirname, 'temp.db')
-        qc.config["core"]["db_debug"] = True


This should not have been commited

astafan8

Very nice! Especially thanks for the refactoring effort.

Regarding examples - we generally want to get rid of arrayparameter and multiparameter, right? so, we should not encourage our users to use them. on the other hand removing them will take some time for use, especially removing multiparameter from all the drivers. hence, the example notebooks should include registering a multiparameter but with a note that those are to be removed or smth.

astafan8 · 2018-08-28T08:35:00Z

qcodes/dataset/measurements.py

-                                     f' Values only given for {params}.')
+                                     f' Values only given for'
+                                     f' {found_parameters}.')
+
        if inserting_unrolled_array and inserting_as_arrays:


inserting_as_arrays and inserting_unrolled_array don't seem to be changed in the code above. are they not needed at all (also, why are tests passing?:) )

inserting_as_arrays is updated in line 162 and inserting_unrolled_array is updated in line 169
I think this is correct and added a test for mixing numeric and array types that raises here

indeed, i didn't look into the folded part so fthe code, sorry. Great!

astafan8 · 2018-08-28T08:35:25Z

qcodes/dataset/measurements.py

+                    self._unbundle_arrayparameter(parameter,
+                                                  res,
+                                                  found_parameters)
+


very nice! looks much cleaner now!

astafan8 · 2018-08-28T08:35:42Z

qcodes/dataset/measurements.py

+    def _append_results(self, res: Sequence[res_type],
+                        input_size: int) -> None:
+        """
+        A private method to add the date to actual queue of data to be written.


astafan8 · 2018-08-28T08:38:14Z

qcodes/dataset/measurements.py

+        setpoint_axes = []
+        setpoint_meta = []
+        if setpoints is None:
+            raise RuntimeError(f"Got an {type(parameter)} without "


perhaps, also add the name of the parameter to the message for clarity?

astafan8 · 2018-08-28T08:40:01Z

qcodes/dataset/measurements.py

+
+    def _unbundle_setpoints_from_param(self, parameter,
+                                       sp_names, fallback_sp_name,
+                                       setpoints, found_parameters, res):


I'm a bit worried that this method changes the given res. I do not think that it is a good pattern. Could you at least mention explicitly that this method (and so far as i see) some other methods are changing the contents of res?

astafan8 · 2018-08-28T08:50:22Z

qcodes/dataset/measurements.py

-            self.parameters.pop(name)
-
-        self.parameters[name] = parspec
+        self._register_individual_parameter(name,


great refactoring!

astafan8 · 2018-08-28T08:52:00Z

qcodes/instrument/parameter.py

+            if inst_name:
+                return tuple(inst_name + '_' + spname for spname in
+                             self.setpoint_names)
+        except AttributeError:


i see that this code is almost copy-pasted from full_names, nevertheless, could you make this cleaner please? are you checking for the _instrument or full_name?

astafan8 · 2018-08-28T08:57:55Z

qcodes/tests/dataset/test_measurement_context_manager.py

+    assert datasaver.points_written == 15
+    ds = load_by_id(datasaver.run_id)
+
+    assert ds.get_data('dummy_channel_inst_ChanA_this_setpoint') == [[5],


what a crap of a function is get_data as it returns a list of single-element lists ....

astafan8 · 2018-08-28T09:00:04Z

qcodes/dataset/measurements.py


        Raises:
            ValueError: if a parameter name not registered in the parent
                Measurement object is encountered.
            ParameterTypeError: if a parameter is given a value not matching
                its type.
        """
-        res = list(res_tuple)  # ArrayParameters cause us to mutate the results
+        # ArrayParameters and MultiParameters cause us to mutate
+        # the results.


is this comment still relevant? you don't seem to mutate results, instead a new "list of resutls" is created now, right? also, does this change impact performance?

Removed the comment.
My preliminary benchmarks indicate that the difference is small as long as the number of tuples added is small. I therefore think this is worth the performance trade off. The alternative would involve removing paramter tuples from the list for multiparameters. That does not seem like a good idea

astafan8 · 2018-08-28T09:01:42Z

qcodes/dataset/measurements.py

@@ -296,7 +398,8 @@ def __init__(
        self.parameters = parameters
        # here we use 5 s as a sane default, but that value should perhaps
        # be read from some config file
-        self.write_period = write_period if write_period is not None else 5
+        self.write_period = float(write_period) \
+            if write_period is not None else 5.0


why is this int/float business needed for write_period? also, if it is needed, just keep it in the write_period.setter only, there is no reason for other methods to care about this.

The existing logic was wrong and only worked because nothing was actually checked by mypy. You cannot use the Number ABC as a type. So I opted for making it float internally. The alternative is just make everything a union[float, int] I don't really care either which way.

ok. now, the solution is fine, it's just that i'd rather have float(..) calls in one place.

…the other one not

jenshnielsen · 2018-08-30T08:54:48Z

I think this addresses @astafan8 comments and now has an example and some more tests. @WilliamHPNielsen and @Dominik-Vogel any opinions.

@nataliejpg I think this should solve your Multi Parameter issue

astafan8

Great!

nataliejpg · 2018-09-03T08:19:30Z

solves multiparameter issue so I am a big fan and very pro merge

Dominik-Vogel · 2018-09-04T11:38:37Z

qcodes/dataset/measurements.py

@@ -90,31 +103,56 @@ def add_result(self,
        are not unraveled but stored directly for improved performance.

        Args:
-            res: a dictionary with keys that are parameter names and items
-                that are the corresponding values at this measurement point.
+            res_tuple: a tuple with the first element being the parameter name


Maybe we should adjust the doctring to say that one can also pass the parameter itself.

Dominik-Vogel

This looks like a very thorough and high quality PR.

Merge: 53cbbaa 83a838d Author: Dominik Vogel <30660470+Dominik-Vogel@users.noreply.github.com> Merge pull request #1249 from jenshnielsen/dataset_multiparameter

jenshnielsen and others added 24 commits August 27, 2018 20:45

add scalar multiparameter to test mock

8204299

Add short_names alias to multiparamter

9ee692f

Add support for registering multiparamters

bd91d8b

Add tests of registering multiparameters

2848636

Remove out of date comment

7571273

Remove now duplicated code

5490700

Add setpoint_full_names

2d7fb33

Refactor ArrayParameter into function

2ada7c8

and make it slightly more readable

refactor add_result to simplify

010d5b9

Refactor to avoid changing list while looping over it

ad18e41

Small cleanup

7777483

Add support for unbundling scalar MultiParameters

114150a

Add support for expanding setpoints of multiparameres with arrays

cebb2bb

add setpoints_full_names to multiparameter

9d83513

test that output is as expected

1e9d850

Mypy now passing

4a14357

Cleanup and docstrings

52e8e12

cleanup docstrings and formatting

a65107b

remove redundant dict

f6af631

fix tests for very large integer write periods

d627243

By ensuring that writeperid is always a float

refactor to reduce code duplication

63648c2

add tests of multidim multiparameter

7f301b2

remove redundant variables

142307e

Merge branch 'master' into dataset_multiparameter

07a4481

jenshnielsen commented Aug 28, 2018

View reviewed changes

astafan8 reviewed Aug 28, 2018

View reviewed changes

astafan8 added enhancement new dataset labels Aug 28, 2018

cleaner full names

e3d4882

jenshnielsen added 6 commits August 29, 2018 11:11

handle the somewhat odd cases of one setpoint axis having a name but …

6e251dd

…the other one not

Add tests for setpoint_names

babe713

Raise if trying to insert scalars into the database

1294637

Add tests for behaviour when mixing arrays and numeric

2394989

More descriptive error

cb51917

rename register_individualparameter

0a6f8aa

jenshnielsen force-pushed the dataset_multiparameter branch from cd852f8 to 0a6f8aa Compare August 29, 2018 11:56

jenshnielsen and others added 8 commits August 29, 2018 15:13

remove out of date todo

26febb4

Unify logic and add some docstrings

d4e28b0

Reorder docstrings

efba58d

Merge branch 'master' into dataset_multiparameter

265ac51

remove out of date comment

02bc496

This always returns a float

54b0f30

Debug level should never have been changed

7874515

Update notebook with examples of MultiParameter

6bbc0a0

jenshnielsen changed the title ~~[wip] Dataset support multiparameter~~ Dataset support multiparameter Aug 30, 2018

astafan8 approved these changes Aug 30, 2018

View reviewed changes

astafan8 mentioned this pull request Aug 30, 2018

String-valued parameters support #1248

Merged

11 tasks

Merge branch 'master' into dataset_multiparameter

efe9446

Dominik-Vogel reviewed Sep 4, 2018

View reviewed changes

Dominik-Vogel approved these changes Sep 4, 2018

View reviewed changes

Merge branch 'master' into dataset_multiparameter

83a838d

Dominik-Vogel merged commit 8a8919b into microsoft:master Sep 4, 2018

giulioungaretti pushed a commit that referenced this pull request Sep 4, 2018

Generated gh-pages for commit 8a8919b

61e8332

Merge: 53cbbaa 83a838d Author: Dominik Vogel <30660470+Dominik-Vogel@users.noreply.github.com> Merge pull request #1249 from jenshnielsen/dataset_multiparameter

jenshnielsen deleted the dataset_multiparameter branch September 5, 2018 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dataset support multiparameter #1249

Dataset support multiparameter #1249

jenshnielsen commented Aug 27, 2018 •

edited

codecov bot commented Aug 28, 2018 •

edited

jenshnielsen Aug 28, 2018

astafan8 left a comment

astafan8 Aug 28, 2018

jenshnielsen Aug 29, 2018

astafan8 Aug 29, 2018

astafan8 Aug 28, 2018

astafan8 Aug 28, 2018

astafan8 Aug 28, 2018

astafan8 Aug 28, 2018

astafan8 Aug 28, 2018

astafan8 Aug 28, 2018

astafan8 Aug 28, 2018

jenshnielsen Aug 29, 2018

astafan8 Aug 28, 2018

jenshnielsen Aug 29, 2018

astafan8 Aug 28, 2018

jenshnielsen Aug 28, 2018

astafan8 Aug 28, 2018

jenshnielsen commented Aug 30, 2018

astafan8 left a comment

nataliejpg commented Sep 3, 2018

Dominik-Vogel Sep 4, 2018

Dominik-Vogel left a comment

Dataset support multiparameter #1249

Dataset support multiparameter #1249

Conversation

jenshnielsen commented Aug 27, 2018 • edited

codecov bot commented Aug 28, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

astafan8 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

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

Choose a reason for hiding this comment

jenshnielsen commented Aug 30, 2018

astafan8 left a comment

Choose a reason for hiding this comment

nataliejpg commented Sep 3, 2018

Choose a reason for hiding this comment

Dominik-Vogel left a comment

Choose a reason for hiding this comment

jenshnielsen commented Aug 27, 2018 •

edited

codecov bot commented Aug 28, 2018 •

edited