Driver/awg5014 #410

WilliamHPNielsen · 2016-12-08T14:24:42Z

Fix: Make AWG5014 driver less of a bloody mess

Changes proposed in this pull request:

Remove some dead methods
Add docstrings to all methods
Add an example notebook

Changes planned for the near future:

Expand example notebook to show how to upload and load .awg files
Make SCPI command syntax uniform (use full command strings)
Delete more dead methods
Refactor a few set/get methods to be parameters
Make helper methods to ease the creation of .awg files

No notebook previously documented how to use the AWG driver. More functions are to be added in a later commit.

This commit removes five methods deemed obsolete from the code. The methods either contained python3 incompatible lines or could not work on a general setup. Removing them improves maintainability of the code.

This commit removes the method `set_filename`. The method contained variable names and comments in Dutch, deeming it hard to maintain. Furthermore, the methods overrode the built-in bool type, which is obviously very dangerous.

Subsistute logging.info, logging.warning, and logging.debug for print statements. Remove the use of the protected name `dir` as a variable (call it `folder` instead)..

This change adds a docstring to each and every method of the driver class.

Make all SCPI commands uniform by using the full mixed-case form also found in the manual. This will make searching the driver easier.

Rewrite all old-style string formatting commands ('Foo is %i' % 100) to modern style ('Foo is {}'.format(100))

…Qcodes into driver/AWG5014

This commit adds a high-level (easy-to-use) function for making and uploading .awg-files as well as an example notebook documenting how to use it.

This commit removes a handful of methods from the AWG5014 driver and crucially also removes positional arguments from the __init__ function. There are now only two supported ways of uploading waveforms. This is described in the example notebook. Breaking changes: the __init__ function now only takes two positional arguments, name and address.

…Qcodes into driver/AWG5014

This commit changes the sequence length, the reference clock state, the DC output state, and the DC output level for each channel from being methods to QCoDeS parameters.

giulioungaretti

Minor notes aside LGTM ! Terrific job !

giulioungaretti · 2016-12-16T13:11:53Z

qcodes/instrument_drivers/tektronix/AWG5014.py

-# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
-
 import numpy as np


The imports do not follow pep8 :D

giulioungaretti · 2016-12-16T13:12:31Z

qcodes/instrument_drivers/tektronix/AWG5014.py

+                message = self.visa_handle.read()
+                if verbose:
+                    print(message)
+            except:


It would be nice to catch the right exception here!

giulioungaretti · 2016-12-16T13:14:54Z

qcodes/instrument_drivers/tektronix/AWG5014.py

        logging.debug(
-            __name__ + ' : Sending waveform %s to instrument' % wfmname)
+            __name__ + ' : Sending waveform {} to instrument'.format(wfmname))


Suggestion for logging:
use a module level logger as in here:
https://github.com/QCoDeS/Qcodes/pull/416/files#diff-4c81a52a7bb6d513e5c5cd6c1b1f2c38R11

then all the calls to the logging system will include the module name which allows to tweak logging (say levels and handlers per module ) 👠

giulioungaretti · 2016-12-16T13:22:05Z

qcodes/instrument_drivers/tektronix/AWG5014.py

+        Delete all user-defined waveforms in the list in a single
+        action. Note that there is no “UNDO” action once the waveforms
+        are deleted. Use caution before issuing this command.
+


trailing whitespace :P

giulioungaretti

Try to check all the returns documentation.

It should look like this:

        Returns:
            dict: A dict with the current setting for each entry in
            AWG_FILE_FORMAT_HEAD iff this entry applies to the AWG5014 AND has
            been changed from its default value.

giulioungaretti · 2016-12-16T13:36:18Z

qcodes/instrument_drivers/tektronix/AWG5014.py

+                           set_cmd='SEQuence:LENGth ' + '{}',
+                           get_parser=int,
+                           vals=vals.Ints(0, 8000),
+                           docstring=('This command sets the sequence length' +


We may want to use a single """ """" string here!

giulioungaretti · 2016-12-16T13:37:09Z

qcodes/instrument_drivers/tektronix/AWG5014.py

+
+        Returns:
+            str: A ""-encapsulated \n-terminated string with the full path
+                of the current folder.


This line should be indented a bit less, else the documentation breaks!
You can check it by running the build doc on your machine as well!

Related to what I suggested last week, I would build the docs and ensure than no Sphinx warnings are raised from this file. RST syntax is hard to get right without testing

giulioungaretti · 2016-12-16T13:37:46Z

qcodes/instrument_drivers/tektronix/AWG5014.py

+
+        Returns:
+            str: Either 'HARD' or 'SOFT' indicating that the instrument is in
+                either hardware or software sequencer mode.


Same as above indent less!

giulioungaretti · 2016-12-16T13:38:03Z

qcodes/instrument_drivers/tektronix/AWG5014.py

+
+        Returns:
+            dict: A dict with the current setting for each entry in
+                AWG_FILE_FORMAT_HEAD iff this entry applies to the


Same as above

AdriaanRol

Really like the work you've done, especially all the docstrings. I've mostly checked the initial part.

Most potential for breaking stuff requires testing. I suggest we wait for @CJvanDiepen 's feedback before merging.

AdriaanRol · 2016-12-16T18:27:19Z

qcodes/instrument_drivers/tektronix/AWG5014.py

@@ -310,29 +337,6 @@ def __init__(self, name, setup_folder, address, reset=False,
                    vals=vals.Numbers(-2.7, 2.7),
                    get_parser=float)

-        # # Add functions


What is the reason for removing the add_functions? As far as I understand this is a QCoDeS concept to easily list all the function (that are note parameters) in an instrument.

I was wondering what happened to this, is the add functions a deprecated qcodes concept?

@AdriaanRol commented code was removed, and in many cases the functions where not even defined. So in all effects this particular change is a NOP 🎉

Functions are still there as methods/parameters or as qcodes.functions where it makes sense to @WilliamHPNielsen (and also according to the documentation of the Function class), see line 143.
I would say that many of the comment lines were pretty good candiate for being parameters.
example

# self.add_function('set_event_jump_timing') # self.add_function('get_event_jump_timing')

became:

self.add_parameter('event_jump_timing', get_cmd='EVENt:JTIMing?', set_cmd='EVENt:JTIMing {}', vals=vals.Enum('SYNC', 'ASYNC'))

Hope this explains !

AdriaanRol · 2016-12-16T18:28:07Z

qcodes/instrument_drivers/tektronix/AWG5014.py

@@ -341,328 +345,426 @@ def __init__(self, name, setup_folder, address, reset=False,

    # Functions
    def get_all(self, update=True):


Why not remove this (or at least add a depreciation warning) and let people use snapshot(update=True)

No good reason. I left it in case of compatibility issues, but I agree that it is a silly wrapper/renaming for a standard function.

A warning has been added.

AdriaanRol · 2016-12-16T18:29:49Z

qcodes/instrument_drivers/tektronix/AWG5014.py

                  .replace(',"$', '\n$').replace('","', '\n')
                  .replace(',', '\t'))
-        return self.ask('mmem:cat?')
+        return self.ask('MMEMory:CATalog?')


Why query twice? Is it not better to ask it once and then print if print_contents=True

Yes, that is better. I didn't go through all the methods to see if they were smart, so there are probably (quite?) a few blunders left. I'll certainly fix this one.

AdriaanRol · 2016-12-16T18:34:49Z

qcodes/instrument_drivers/tektronix/AWG5014.py

-        print('Current AWG file set to: ', self.get_current_folder_name())
-        self.write('AWGC:SRES "%s.awg"' % fname)
-
-    def set_setup_filename(self, fname, force_load=False):


I'm not sure if removing this is a good idea. Knowing about the hardware limitations (writing speed) it is sometimes very convenient to load a sequence that was previously put in memory of the 5014. I think this function can be cleaned up a bit but removing it completely may not be such a good idea.

My idea was that it is better to have just one way of doing things. To load an awg file from memory (which is not a stupid thing to do for large files where file transfer times matter), one can/should now use load_awg_file. Do you think that set_setup_filename does something very different than load_awg_file? If so, I would suggest to incorporate that into load_awg_file.

CJvanDiepen · 2017-01-02T10:23:24Z

@AdriaanRol @WilliamHPNielsen Just tested if the functionality of the AWG driver that we use (the send_waveform_to_list style) still works and it does! Only had to adjust the initialization because the revised driver does not work with a setup_folder anymore. Actually we did not use this but I also do not know if the functionality it offered was important, do any of you know?

AdriaanRol · 2017-01-02T13:57:41Z

Hi William, it is quite important to be able to load files from the AWG memory, as some take quite some time to generate. For this purpose the setup folder sets what folder to look in. The name may not be the best but we do use it. In the same line of thought, can we still load a file from a filename?

…

On Mon, 2 Jan 2017 at 03:23, CJvanDiepen ***@***.***> wrote: @AdriaanRol <https://github.com/AdriaanRol> @WilliamHPNielsen <https://github.com/WilliamHPNielsen> Just tested if the functionality of the AWG driver that we use (the send_waveform_to_list style) still works and it does! Only had to adjust the initialization because the revised driver does not work with a setup_folder anymore. Actually we did not use this but I also do not know if the functionality it offered was important, do any of you know? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#410 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF271Ikz-C_85p037a7Pte6JTxF4-meGks5rONAcgaJpZM4LH3zE> .

This commit fixes the issues raised by helpful code reviewers on 2016-12-16.

…Qcodes into driver/AWG5014

This commit adds the possibility to select specific channels to upload waveforms to.

This commit changes the logging to a module level logger.

…Qcodes into driver/AWG5014

nataliejpg · 2017-01-03T22:48:18Z

especially love the example notebook, thanks William.

Add a helper function that can parse an awg file and return numpy arrays of waveforms and markers, lists of sequencer information, and a dictionary with instrument settings.

Introduce a parser to fix a newline problem

Fix a problem with newline-termination of instrument return values by adding a parser.

this covers some of the request made years ago. Because of the design of many pieces of qcodes the code is really hacky and anti-cool.

feature: Add wrappers

Uses empty names and shapes. Will it fix?

Add a parameter for the sequencer position and update the example notebook. Breaking changes: The two methods get_sq_position and sq_forced_jump no longer exist. Their functionality has been absorbed by the new parameter.

…nto driver/AWG5014

…Qcodes into driver/AWG5014

Get in line with the new parameter styles.

jenshnielsen · 2017-02-09T08:44:55Z

replaced by #482

WilliamHPNielsen added 7 commits December 6, 2016 10:39

Starting point.

c628989

feat: Add small (unfinished) example notebook

edbb577

No notebook previously documented how to use the AWG driver. More functions are to be added in a later commit.

refactor: Delete old and unused methods

2eff7aa

This commit removes five methods deemed obsolete from the code. The methods either contained python3 incompatible lines or could not work on a general setup. Removing them improves maintainability of the code.

refactor: Delete semi-broken method

3eb0937

This commit removes the method `set_filename`. The method contained variable names and comments in Dutch, deeming it hard to maintain. Furthermore, the methods overrode the built-in bool type, which is obviously very dangerous.

style: Clean up code (part 1)

908aa55

Subsistute logging.info, logging.warning, and logging.debug for print statements. Remove the use of the protected name `dir` as a variable (call it `folder` instead)..

style: Add docstrings to all the class methods

ca08b5c

This change adds a docstring to each and every method of the driver class.

Merge branch 'master' into driver/AWG5014

fc4702b

WilliamHPNielsen requested a review from giulioungaretti December 8, 2016 14:31

WilliamHPNielsen and others added 9 commits December 9, 2016 11:05

style: Clean up code (part 2)

da31f0a

Make all SCPI commands uniform by using the full mixed-case form also found in the manual. This will make searching the driver easier.

style: Clean up code (part 3)

b95fb32

Rewrite all old-style string formatting commands ('Foo is %i' % 100) to modern style ('Foo is {}'.format(100))

Merge branch 'driver/AWG5014' of https://github.com/WilliamHPNielsen/…

3031378

…Qcodes into driver/AWG5014

feat: Add high-level .awg-file creator

6e87d6e

This commit adds a high-level (easy-to-use) function for making and uploading .awg-files as well as an example notebook documenting how to use it.

Merge branch 'master' into driver/AWG5014

e025668

Merge branch 'driver/AWG5014' of https://github.com/WilliamHPNielsen/…

16738ea

…Qcodes into driver/AWG5014

refactor: Change methods to parameters

8e9780b

This commit changes the sequence length, the reference clock state, the DC output state, and the DC output level for each channel from being methods to QCoDeS parameters.

Merge branch 'master' into driver/AWG5014

57f28cd

giulioungaretti suggested changes Dec 16, 2016

View reviewed changes

AdriaanRol approved these changes Dec 16, 2016

View reviewed changes

WilliamHPNielsen added 6 commits January 2, 2017 17:04

fix: Respond to code reviews

720712a

This commit fixes the issues raised by helpful code reviewers on 2016-12-16.

Merge branch 'driver/AWG5014' of https://github.com/WilliamHPNielsen/…

7ff7ba0

…Qcodes into driver/AWG5014

Merge branch 'master' into driver/AWG5014

2c0f14a

feat: Add channel selection feature

8bfc7c9

This commit adds the possibility to select specific channels to upload waveforms to.

feat: Swicth to module level logger

f1a9700

This commit changes the logging to a module level logger.

Merge branch 'driver/AWG5014' of https://github.com/WilliamHPNielsen/…

03e104b

…Qcodes into driver/AWG5014

WilliamHPNielsen closed this Jan 3, 2017

WilliamHPNielsen reopened this Jan 3, 2017

feat: Add an awg-file parser

854a3a2

Add a helper function that can parse an awg file and return numpy arrays of waveforms and markers, lists of sequencer information, and a dictionary with instrument settings.

jenshnielsen mentioned this pull request Jan 12, 2017

Docs/params #403

Merged

WilliamHPNielsen and others added 15 commits January 17, 2017 09:39

fix: Fix a newline problem (1/2)

6f595fc

Introduce a parser to fix a newline problem

fix: newline problem (2/2)

b09b892

Fix a problem with newline-termination of instrument return values by adding a parser.

feature: Add wrappers

9819686

this covers some of the request made years ago. Because of the design of many pieces of qcodes the code is really hacky and anti-cool.

Merge pull request #2 from qdev-dk/feature/qdev-wrapper

09cc1af

feature: Add wrappers

fix: Include last lash in path

7ff8ad7

hotfix: ZNB20 update after disaster PR

e4bee3e

Uses empty names and shapes. Will it fix?

remove stupid unit

3311ab2

simplify legend

627b2f3

moar simplification

a8b1928

Merge branch 'master' into driver/AWG5014

0537d09

Merge branch 'master' into driver/AWG5014

148df9c

feat: Add sequence position parameter

cdabf11

Add a parameter for the sequencer position and update the example notebook. Breaking changes: The two methods get_sq_position and sq_forced_jump no longer exist. Their functionality has been absorbed by the new parameter.

Merge branch 'master' of https://github.com/WilliamHPNielsen/Qcodes i…

d187393

…nto driver/AWG5014

Merge branch 'driver/AWG5014' of https://github.com/WilliamHPNielsen/…

ef3aa7d

…Qcodes into driver/AWG5014

fix: units -> unit

ef865bf

Get in line with the new parameter styles.

jenshnielsen mentioned this pull request Feb 8, 2017

Driver/awg5014 replacement #482

Merged

WilliamHPNielsen closed this Feb 8, 2017

WilliamHPNielsen deleted the driver/AWG5014 branch February 8, 2017 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Driver/awg5014 #410

Driver/awg5014 #410

WilliamHPNielsen commented Dec 8, 2016 •

edited

Loading

giulioungaretti left a comment

giulioungaretti Dec 16, 2016

giulioungaretti Dec 16, 2016

giulioungaretti Dec 16, 2016

giulioungaretti Dec 16, 2016

giulioungaretti left a comment

giulioungaretti Dec 16, 2016

giulioungaretti Dec 16, 2016 •

edited

Loading

jenshnielsen Dec 16, 2016

giulioungaretti Dec 16, 2016

giulioungaretti Dec 16, 2016 •

edited

Loading

giulioungaretti Dec 16, 2016

AdriaanRol left a comment

AdriaanRol Dec 16, 2016

AdriaanRol Jan 2, 2017

giulioungaretti Jan 2, 2017

AdriaanRol Dec 16, 2016

WilliamHPNielsen Jan 2, 2017 •

edited

Loading

AdriaanRol Dec 16, 2016

WilliamHPNielsen Jan 2, 2017

AdriaanRol Dec 16, 2016

WilliamHPNielsen Jan 2, 2017 •

edited

Loading

CJvanDiepen commented Jan 2, 2017

AdriaanRol commented Jan 2, 2017 via email

nataliejpg commented Jan 3, 2017 •

edited

Loading

jenshnielsen commented Feb 9, 2017

		@@ -341,328 +345,426 @@ def __init__(self, name, setup_folder, address, reset=False,

		# Functions
		def get_all(self, update=True):

Driver/awg5014 #410

Driver/awg5014 #410

Conversation

WilliamHPNielsen commented Dec 8, 2016 • edited Loading

giulioungaretti 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

giulioungaretti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giulioungaretti Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giulioungaretti Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdriaanRol 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

WilliamHPNielsen Jan 2, 2017 • 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

WilliamHPNielsen Jan 2, 2017 • edited Loading

Choose a reason for hiding this comment

CJvanDiepen commented Jan 2, 2017

AdriaanRol commented Jan 2, 2017 via email

nataliejpg commented Jan 3, 2017 • edited Loading

jenshnielsen commented Feb 9, 2017

WilliamHPNielsen commented Dec 8, 2016 •

edited

Loading

giulioungaretti Dec 16, 2016 •

edited

Loading

giulioungaretti Dec 16, 2016 •

edited

Loading

WilliamHPNielsen Jan 2, 2017 •

edited

Loading

WilliamHPNielsen Jan 2, 2017 •

edited

Loading

nataliejpg commented Jan 3, 2017 •

edited

Loading