driver / SR830 : add the snap XY command to read X and Y together #1333

GeneralSarsby · 2018-10-23T14:45:03Z

The snap command records the values of X and Y at a single instant. This is a way to query values at the same time. This is important when the time constant is very short, < 100 ms.
-> tuple (x,y)

Changes proposed in this pull request:

add a single parameter, SNAP_XY

The snap command records the values of X and Y at a single instant. This is a way to query values at the same time. This is important when the time constant is very short, < 100 ms. -> tuple (x,y)

typographical error.

astafan8

This is a great addition! Nevertheless,

please make it a method rather than a parameter since it is not settable, and returns two values
is "X,Y" combination the only one that the device can return?
please use lowercase "snake_case" names for parameters to be in accordance with the default style guide for python

the snap_xy is now a function using "add_function". in turn get_cmd -> call_cmd, and get_parser -> return_parser. SNAP_XY is also now lowercase

GeneralSarsby · 2018-10-23T16:46:48Z

Hi astafan8,

I've put the changes you recommended. It is now a function and in snake_case.

the "X,Y" is not the only combination, but it the most important of the snap commands.

In brief:
The snap command can return up to six different values at the same time.
This is up to 6, in any order, from 11 different choice values that the lockin can measure, including the phase, Aux1, freq, etc.

This single snap_xy function I think covers the 99% of the use cases.
However I can see that a more general function would be something like:
lockin.snap('x','y','freq','aux1') -> (x,y,freq,aux1)

What do you think? make it generalized? or simple?

astafan8 · 2018-10-23T17:29:22Z

qcodes/instrument_drivers/stanford_research/SR830.py

@@ -386,7 +386,17 @@ def parse_offset_get(s):
                           get_cmd='OUTP? 4',
                           get_parser=float,
                           unit='deg')
-
+
+        self.add_function('snap_xy',


I meant class bound method, like def snap_xy(...): ..... Sorry that I was not clear enough.

astafan8 · 2018-10-23T17:34:50Z

@GeneralSarsby Yes, indeed, since you are implementing this great and useful feature, please, implement it in a flexible way. You can refer to this implementation in SR860 as a reference.

Also, users will be very thankful if you also update the example notebook for SR830 with information on this feature (see the example notebook for SR860 for an example).

I'm sure that there will be users who will fall into the 1% of the cases that snap_xy does not cover, and they will be very happy to know that there is a general snap (or whatever well-named) method that allows them to utilize the full power of SR830 and return up to 6 synchronized values at once out of 11 available.

codecov · 2018-10-23T22:49:01Z

Codecov Report

Merging #1333 into master will decrease coverage by 0.92%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1333      +/-   ##
==========================================
- Coverage   73.15%   72.22%   -0.93%     
==========================================
  Files          79       83       +4     
  Lines        9156     9566     +410     
==========================================
+ Hits         6698     6909     +211     
- Misses       2458     2657     +199

snap is now a class bound method. it can be used as lockin.snap('x','y') -> tuple(x,y) it checks for a correct number of parameters and that the parameters are valid. It is not fussy about 'x' or 'X', and the confusing phase option can be 'p', 'P', 'Phase', 'phase', or 'θ'. to help with compatibility between similar drivers.

astafan8

Great, a flexible snap method is much better. But please respond to my comments. Also, i'd really encourage you to document this feature in the example notebook (let me know if you need help with that).

astafan8 · 2018-10-24T14:25:13Z

qcodes/instrument_drivers/stanford_research/SR830.py

+            mands will result in time delays, which may be greater than the time con-
+            stant, between reading X and Y (or R and θ)
+            Thus reading  X,Y  OR  R,θ yields  a  coherent  snapshot  of  the  output  signal.
+


it's great that this corresponds to the description in the manual. However, i think it is also important that the following is also mentioned here:

The values of X and Y are recorded at a single instant. The values of R and θ are also recorded at a single instant. Thus reading X,Y OR R,θ yields a coherent snapshot of the output signal. If X,Y,R and θ are all read, then the values of X,Y are recorded approximately 10µs apart from R,θ. Thus, the values of X and Y may not yield the exact values of R and θ from a single SNAP? query. The values of the Aux Inputs may have an uncertainty of up to 32µs. The frequency is computed only every other period or 40 ms, whichever is longer. The SNAP? command is a query only command. The SNAP? command is used to

astafan8 · 2018-10-24T14:25:48Z

qcodes/instrument_drivers/stanford_research/SR830.py

+    def snap(self, *parameter_names: str) -> Tuple[float, ...]:
+            """
+            Gets the values of either 2, 3, 4, 5 or 6 parameters at a single instant.
+            For example, SNAP? is a way to query values of


Please, substitute the notions of exact visa commands which they parameters/methods from the driver. We developer drivers exactly for the reason of hiding all of this visa-level stuff.

astafan8 · 2018-10-24T14:27:01Z

qcodes/instrument_drivers/stanford_research/SR830.py

+    def snap(self, *parameter_names: str) -> Tuple[float, ...]:
+            """
+            Gets the values of either 2, 3, 4, 5 or 6 parameters at a single instant.
+            For example, SNAP? is a way to query values of


"for example"? this does not read nicely ((

I've tried to re-write the whole docstring to make things clearer. Both with what the function does and with the limitations. I think this also covers a previous comment. We can develop this further if needed.

astafan8 · 2018-10-24T14:27:24Z

qcodes/instrument_drivers/stanford_research/SR830.py

+
+            Args:
+                *parameter_names
+                    2 or 3 names of parameters for which the values are


2 or 3? it should be "from 2 up to 6", right?

astafan8 · 2018-10-24T14:28:43Z

qcodes/instrument_drivers/stanford_research/SR830.py

+            Args:
+                *parameter_names
+                    2 or 3 names of parameters for which the values are
+                    requested; valid names can be found in `PARAMETER_NAMES`


typo: SNAP_PARAMETERS?

qcodes/instrument_drivers/stanford_research/SR830.py

astafan8 · 2018-10-24T14:31:26Z

qcodes/instrument_drivers/stanford_research/SR830.py

+            Gets the values of either 2, 3, 4, 5 or 6 parameters at a single instant.
+            For example, SNAP? is a way to query values of
+            X and Y (or R and θ) which are taken at the same time. This is important
+            when the time constant is very short. Using the OUTP? or OUTR? com-


within qcodes, we are generally following pep8, which requires the lines of code to not exceed 80 characters. Could you please adjust the code accordingly?

Yep. This can be done.

astafan8 · 2018-10-24T14:32:52Z

qcodes/instrument_drivers/stanford_research/SR830.py

+                                   f'to `SNAP_PARAMETERS` for a list of valid '
+                                   f'parameter names')
+
+            p_ids = [self.PARAMETER_NAMES[name.lower()] for name in parameter_names]


typo: perhaps SNAP_PARAMETERS?

(have this change been tested after all? :) )

Rewritten the doc string cleaning up the text. Removed all notions of visa commands. Removed space from 'aux 4' -> 'aux4' and similar in the SNAP_PARAMETERS. All lines are less than 80 characters long. To follow PEP8. Fixed typos.

astafan8

Now i think it's ready :) Assuming that it has been tested on the actual instrument.

astafan8 · 2018-10-25T12:42:49Z

qcodes/instrument_drivers/stanford_research/SR830.py

+
+    def snap(self, *parameters: str) -> Tuple[float, ...]:
+        """
+Get between 2 and 6 parameters at a single instant. This provides a coherent


so far as i know the text for the docstring shall start from """, like this:

def smth(): """ a great docstring ...

In addition our docstrings should be valid restructured text using the Google docs extension see https://qcodes.github.io/Qcodes/examples/Creating%20Instrument%20Drivers.html#Documentation and https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

Specifically the args, returns and example sections should conform to the standard and you are not allowed to create other sections like the Units and Limitations. You probably want to put this in a notes section

astafan8 · 2018-11-07T12:26:31Z

@GeneralSarsby When you confirm that the feature has been tested on the actual instrument, I am going to merge. Looking forward to hearing from you

qcodes/instrument_drivers/stanford_research/SR830.py

included Tuple from the typing module. fixed the docstring indent. rearranged the section headings in the docstring.

Merge: d6564c6 4ea67f9 Author: Mikhail Astafev <astafan8@gmail.com> Merge pull request #1333 from GeneralSarsby/patch-1

GeneralSarsby added 2 commits October 23, 2018 16:43

added the snap XY command to read X and Y together

25d6150

The snap command records the values of X and Y at a single instant. This is a way to query values at the same time. This is important when the time constant is very short, < 100 ms. -> tuple (x,y)

Update SR830.py

c69bb93

typographical error.

astafan8 requested changes Oct 23, 2018

View reviewed changes

changed the parameter to a function ...

f48618d

the snap_xy is now a function using "add_function". in turn get_cmd -> call_cmd, and get_parser -> return_parser. SNAP_XY is also now lowercase

astafan8 reviewed Oct 23, 2018

View reviewed changes

WilliamHPNielsen changed the title ~~Added the snap XY command to read X and Y together~~ driver / SR830 : add the snap XY command to read X and Y together Oct 24, 2018

WilliamHPNielsen added the driver label Oct 24, 2018

astafan8 requested changes Oct 24, 2018

View reviewed changes

Update SR830.py

aa07256

Rewritten the doc string cleaning up the text. Removed all notions of visa commands. Removed space from 'aux 4' -> 'aux4' and similar in the SNAP_PARAMETERS. All lines are less than 80 characters long. To follow PEP8. Fixed typos.

astafan8 approved these changes Oct 25, 2018

View reviewed changes

Merge branch 'master' into patch-1

bd11218

WilliamHPNielsen reviewed Nov 8, 2018

View reviewed changes

qcodes/instrument_drivers/stanford_research/SR830.py Show resolved Hide resolved

Update SR830.py

4ea67f9

included Tuple from the typing module. fixed the docstring indent. rearranged the section headings in the docstring.

astafan8 merged commit ffc6f06 into microsoft:master Nov 27, 2018

giulioungaretti pushed a commit that referenced this pull request Nov 27, 2018

Generated gh-pages for commit ffc6f06

9287d47

Merge: d6564c6 4ea67f9 Author: Mikhail Astafev <astafan8@gmail.com> Merge pull request #1333 from GeneralSarsby/patch-1

GeneralSarsby deleted the patch-1 branch November 27, 2018 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

driver / SR830 : add the snap XY command to read X and Y together #1333

driver / SR830 : add the snap XY command to read X and Y together #1333

GeneralSarsby commented Oct 23, 2018

astafan8 left a comment

GeneralSarsby commented Oct 23, 2018

astafan8 Oct 23, 2018

astafan8 commented Oct 23, 2018

codecov bot commented Oct 23, 2018 •

edited

astafan8 left a comment

astafan8 Oct 24, 2018

astafan8 Oct 24, 2018

astafan8 Oct 24, 2018

GeneralSarsby Oct 25, 2018

astafan8 Oct 24, 2018

astafan8 Oct 24, 2018

astafan8 Oct 24, 2018

GeneralSarsby Oct 25, 2018

astafan8 Oct 24, 2018

astafan8 left a comment

astafan8 Oct 25, 2018

jenshnielsen Nov 8, 2018

astafan8 commented Nov 7, 2018

driver / SR830 : add the snap XY command to read X and Y together #1333

driver / SR830 : add the snap XY command to read X and Y together #1333

Conversation

GeneralSarsby commented Oct 23, 2018

astafan8 left a comment

Choose a reason for hiding this comment

GeneralSarsby commented Oct 23, 2018

Choose a reason for hiding this comment

astafan8 commented Oct 23, 2018

codecov bot commented Oct 23, 2018 • edited

Codecov Report

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

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

astafan8 commented Nov 7, 2018

codecov bot commented Oct 23, 2018 •

edited