Rigol dg1062 #1082

sohailc · 2018-05-04T12:19:48Z

A driver for the Rigol DG1062

…dg1062

codecov · 2018-05-04T13:26:52Z

Codecov Report

Merging #1082 into master will decrease coverage by <.01%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #1082      +/-   ##
==========================================
- Coverage   79.78%   79.77%   -0.01%     
==========================================
  Files          48       48              
  Lines        6657     6664       +7     
==========================================
+ Hits         5311     5316       +5     
- Misses       1346     1348       +2

jenshnielsen

Left a few minor inline comments but looks fine otherwise

jenshnielsen · 2018-05-04T14:26:59Z

qcodes/instrument_drivers/rigol/DG1062.py

+        super().__init__(name, address, terminator="\n", **kwargs)
+
+        channels = ChannelList(self, "channel", DG1062Channel,
+                               snapshotable=True)


You may want to set this false? I think as it is the channels will be in the snapshot twice

jenshnielsen · 2018-05-04T14:30:11Z

qcodes/instrument_drivers/rigol/DG1062.py

+
+        super().__init__(parent, name)
+
+        self.parent = parent


This looks redundant to me. I would prefer to add a parent property to the baseclass. We should have done that a long time ago to not have to call _parent

Of course, you can remove line 29.

Right now it seems we do need both lines. Line 27 does not automatically add parent to self.

It adds it but only as _parent which IMHO is a bug in the Channel class. There should be a non private way to get the parent so I would add a parent property to the Channel clase

@property def parent(self): return self.parent

Note that we already have a root instrument property. Perhaps we can use that here

jenshnielsen · 2018-05-07T08:10:52Z

qcodes/instrument_drivers/rigol/DG1062.py

+
+        Example:
+        >>> gd = DG1062("gd", "TCPIP0::169.254.187.99::inst0::INSTR")
+        >>> gd.channels[0].apply(waveform="SIN", freq=1E3, ampl=1.0, offset=0, phase=0)


Is there a good way to know the valid arguments. Perhaps even just a ref to the relevant page in a manual

The valid arguments depend on the waveform, which makes this a bit tricky. If you give wrong arguments I raise a ValueError and list the arguments expected. See the example notebook.

Perhaps I can build an alternate interface which looks something like:

gd.channel[0].apply.sin(freq, ampl, offset, phase)

The advantage of that is that we can then have proper doc strings which the user can refer to.

…dg1062

… driver/rigol_dg_1062_feature_add

Added sync param

…/rigol_dg_1062_feature_add

…ough the help built-in 2) Made an acceptable example notebook

…/rigol_dg_1062_feature_add

sohailc · 2018-07-20T09:53:54Z

@jenshnielsen can you have another look at this, please?

astafan8 · 2018-07-20T09:58:32Z

qcodes/instrument_drivers/rigol/DG1062.py

+log = logging.getLogger(__name__)
+
+
+def sane_partial(func, docstring, **kwargs):


shall this reside in "helpers" somewhere?

Yeah that could be helpful. Where exactly would you propose to put this?

qcodes/utils/helpers.py should be an OK place. Although that file is already huge...

astafan8 · 2018-07-20T10:00:27Z

qcodes/instrument_drivers/rigol/DG1062.py

+        self.add_parameter(
+            "polarity",
+            get_cmd=f":SOUR{channel}:BURS:GATE:POL?",
+            set_cmd=f":SOUR{channel}:BURS:GATE:POL {{}}",


just for my understanding, why are double brackets needed here?

https://www.python.org/dev/peps/pep-0498/#specification

f"{{}}" gives you "{}"

that i know, just haven't seen visa commands which contain brackets for the values ;)

Nor do you see that here, it's a string to be formatted later by Command. See https://github.com/QCoDeS/Qcodes/blob/master/qcodes/utils/command.py

So here we want to have set_cmd=":SOUR1:BURS:GATE:POL {}" etc.

astafan8 · 2018-07-20T10:01:10Z

qcodes/instrument_drivers/rigol/DG1062.py

+
+    def trigger(self) ->None:
+        """
+        Send a soft trigger to the instrument. This only works if the trigger


"soft" means "from software"/"from the driver"?

Yes. I will make this more clear

jenshnielsen · 2018-07-23T09:20:23Z

qcodes/utils/helpers.py

@@ -581,3 +582,32 @@ def attribute_set_to(object_: Any, attribute_name: str, new_value: Any):
    setattr(object_, attribute_name, new_value)
    yield
    setattr(object_, attribute_name, old_value)
+
+
+def sane_partial(func, docstring, **kwargs):


Could you give this a more descriptive name. Sane_partial tells very little about why this is more sane

jenshnielsen · 2018-07-23T09:38:03Z

@sohailc Looks good but I think we should find a better name for sane_partial

WilliamHPNielsen · 2018-07-23T10:29:56Z

partial_with_docstring? That's at least immediately understandable.

sohailc · 2018-07-23T11:29:08Z

Ok, I will change it to partial_with_docstring and merge the code

Merge: cad7e14 7c0571e Author: sohail chatoor <schatoor@gmail.com> Merge pull request #1082 from sohailc/rigol_dg1062

sohailc added 8 commits May 3, 2018 14:34

Added a basic DG1062 driver

90a7333

Merge branch 'master' of https://github.com/QCoDeS/Qcodes into rigol_…

b85ac2d

…dg1062

some type hinting

ec3e77e

finished hinting and documenting

77ec5d8

Merge branch 'master' of https://github.com/QCoDeS/Qcodes into rigol_…

2901c97

…dg1062

added example notebook

478a5df

fix typo in names: GD1062 -> DG1062

86e040f

mypy should not complain anymore

a4ec620

sohailc added 2 commits May 4, 2018 15:53

mypy should not complain anymore and everything should be working again

9aaf87a

remove List from typing import

be5f61f

sohailc requested a review from jenshnielsen May 4, 2018 14:16

Merge branch 'master' into rigol_dg1062

7d5a69c

jenshnielsen requested changes May 7, 2018

View reviewed changes

sohailc and others added 13 commits May 7, 2018 14:15

more convinient way of applying waveforms.

34afb79

slightly better docstring on apply

0d145fd

updated notebook

48e7e8e

added on/off

c5ff89f

Merge branch 'master' of https://github.com/QCoDeS/Qcodes into rigol_…

2ffbd86

…dg1062

Merge branch 'master' into rigol_dg1062

6204b0f

Merge branch 'rigol_dg1062' of https://github.com/sohailc/Qcodes into…

80288b2

… driver/rigol_dg_1062_feature_add

Added HighZ option to impedance

658c059

Added sync param

Merge branch 'master' of https://github.com/QCoDeS/Qcodes into driver…

e20fd1b

…/rigol_dg_1062_feature_add

Triggering seems to work

7a94c58

1) The docstring of individual waveform functions can be accessed thr…

957c945

…ough the help built-in 2) Made an acceptable example notebook

Merge branch 'master' of https://github.com/QCoDeS/Qcodes into driver…

397ef79

…/rigol_dg_1062_feature_add

Setting snapshotable to False for channels

6060b48

astafan8 reviewed Jul 20, 2018

View reviewed changes

sohailc added 7 commits July 20, 2018 14:02

mypy passes

ada3894

Merge branch 'master' into rigol_dg1062

d880aca

update trigger docstring

622049f

Inshallah CI will pass now

dc4b696

removed unused module union

8ed7075

refactored sane_partial into utils/helpers.py

97fd085

adding newline to helpers.py

209026b

astafan8 approved these changes Jul 20, 2018

View reviewed changes

Make a test for the sane partial function

890458a

jenshnielsen reviewed Jul 23, 2018

View reviewed changes

jenshnielsen approved these changes Jul 23, 2018

View reviewed changes

sohailc added 3 commits July 23, 2018 13:31

name change: sane_partial -> partial_with_docstring

981c121

name change: sane_partial -> partial_with_docstring. Also in test

da44dad

Merge branch 'master' into rigol_dg1062

7c0571e

sohailc merged commit 8c009af into microsoft:master Jul 23, 2018

giulioungaretti pushed a commit that referenced this pull request Jul 23, 2018

Generated gh-pages for commit 8c009af

fbe98f7

Merge: cad7e14 7c0571e Author: sohail chatoor <schatoor@gmail.com> Merge pull request #1082 from sohailc/rigol_dg1062

sohailc deleted the rigol_dg1062 branch October 3, 2018 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rigol dg1062 #1082

Rigol dg1062 #1082

sohailc commented May 4, 2018

codecov bot commented May 4, 2018 •

edited

jenshnielsen left a comment

jenshnielsen May 4, 2018

jenshnielsen May 4, 2018

sohailc May 7, 2018

sohailc May 7, 2018

jenshnielsen May 7, 2018

jenshnielsen May 7, 2018

sohailc May 7, 2018

sohailc May 7, 2018

astafan8 Jul 20, 2018

sohailc commented Jul 20, 2018

astafan8 Jul 20, 2018

sohailc Jul 20, 2018

astafan8 Jul 20, 2018

astafan8 Jul 20, 2018

WilliamHPNielsen Jul 20, 2018

astafan8 Jul 20, 2018

WilliamHPNielsen Jul 23, 2018

astafan8 Jul 20, 2018

sohailc Jul 20, 2018

jenshnielsen Jul 23, 2018

jenshnielsen commented Jul 23, 2018

WilliamHPNielsen commented Jul 23, 2018

sohailc commented Jul 23, 2018

		log = logging.getLogger(__name__)


		def sane_partial(func, docstring, **kwargs):

Rigol dg1062 #1082

Rigol dg1062 #1082

Conversation

sohailc commented May 4, 2018

codecov bot commented May 4, 2018 • edited

Codecov Report

jenshnielsen 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

sohailc commented Jul 20, 2018

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 Jul 23, 2018

WilliamHPNielsen commented Jul 23, 2018

sohailc commented Jul 23, 2018

codecov bot commented May 4, 2018 •

edited