Add support for array parameters to MultiChannelInstrumentParameter by jenshnielsen · Pull Request #2 · spauka/Qcodes

jenshnielsen · 2017-06-08T11:47:40Z

Changes proposed in this pull request:

Add support for ArrayParameters to MultiChannelInstrumentParameter
Error cleanly if tying to use a MultiParameter i.e. slice a MultiChannelInstrumentParameter
Some relevant tests

And error clearly if you try to measure multiple multiparameters

This reverts commit 2dbacc0.

Add a QDac driver using the new channelisation feature

…array Feat/channelization with array

Make the paramclass returned by __getattr__ be customisable.

Add a channelised driver for the QDac. This requires changing the base code for channels a bit to allow for custom MultiChannelInstrumentParameter subclasses.

…array Driver/QDac_channels

spauka

Hey,

Changes look good and hopefully it was reasonably intuitive to convert the QDac into a channelized instrument. A few minor points raised in the review, although I'm not too fussed about any of them.

One suggestion for improving the readability of code given the 1-indexing of channels, if functions such as _set_voltage are part of the QDacChannel then it is possible to use the channum set for each class rather than having to do chan-1 repeatedly.

Something like:

class QDacChannel(InstrumentChannel):
  ...
  def __init__(self, parent, name, channum):
    ...
    self._channum = channum

  def _set_voltage(self, v_set):
    ...
    atten = self.vrange.get_latest() 
    ...
    self.write('set {} {:.6f}'.format(self._channum, v_set)

Perhaps you want to add single channel indexing examples to the notebook, to demonstrate the equivalence between qdac.channels[0] and qdac.ch01.

spauka · 2017-06-15T11:33:08Z

qcodes/instrument_drivers/QDev/QDac_channels.py

+                                   label=label,
+                                   unit='C',
+                                   get_cmd='tem {} {}'.format(board, sensor),
+                                   get_parser=self._num_verbose)


Should these be channelized as well?
Are there more parameters that are board specific, similar to slots in the Harvard DAC?

They could be channelised, but noone uses them, so I think it's okay to leave them for now.

spauka · 2017-06-15T11:43:45Z

qcodes/instrument_drivers/QDev/QDac_channels.py

+                                                      self.visa_handle.read()))
+
+        # take care of the rest of the output
+        for ii in range(50):


should reference self.num_chans?

Or maybe not... It's not so clear why this output is exactly 50 lines long (firmware design choice), but it will not change with self.num_chans. But I agree that magic numbers should be avoided.

spauka · 2017-06-15T11:43:58Z

qcodes/instrument_drivers/QDev/QDac_channels.py

+        self.write('status')
+        FW_str = self._write_response
+        FW_version = float(FW_str.replace('Software Version: ', ''))
+        for ii in range(50):


self.num_chans here too?

No magic numbers. Let's make a variable for this.

spauka · 2017-06-15T11:48:42Z

qcodes/instrument_drivers/QDev/QDac_channels.py

+
+    #########################
+    # Channel gets/sets
+    #########################


Should channel get/set be part of the QDacChannel class?

I think it makes sense for this particular instrument to keep it in the main instrument, as the channels are interlinked via the _get_status command.

spauka · 2017-06-15T11:51:01Z

qcodes/instrument/channel.py

+            stored inside a channel list are accessible in multiple
+            ways and should not be repeated in an instrument snapshot.
+
+        paramclass (unknown): The class of the object to be returned by


unknown can be MultiChannelInstrumentParameter

Good point.

spauka · 2017-06-15T11:54:23Z

qcodes/instrument/channel.py

-    def __init__(self, parent, name, chan_type, chan_list=None, snapshotable=True):
+    def __init__(self, parent, name, chan_type, chan_list=None,
+                 snapshotable=True,
+                 paramclass=MultiChannelInstrumentParameter):


paramclass is a slightly confusing variable name here. Perhaps something like multichan_paramclass. Given that this might be a slightly confusing concept for users I would be in favor of a more verbose variable name.

I fully agree with you on this one.

jenshnielsen · 2017-06-15T12:56:30Z

@spauka Thanks, cant wait to get the channel support merged.

I have another branch at https://github.com/jenshnielsen/qcodes/tree/feat/channelization_refactor_classes which implements 1 based indexing into the channels along with some refactoring. We choose to split these things up as what is in this pr is hopefully fairly non controversial, but the changes in the other PR will require some more thought and should perhaps be changed and/or removed.

On top of that I did a version of microsoft#623 on top of the channels in https://github.com/jenshnielsen/qcodes/tree/znb_with_channels This looks very promising for the channels as it greatly simplifies the driver implementation (no more get_attr) and makes the the driver much easier to use (I expect, it's currently untested as I don't have access to a spare RS ZNB 20)

jenshnielsen · 2017-06-15T13:38:07Z

@spauka I pushed some changes to address the issues per Williams suggestions

spauka · 2017-06-16T00:05:46Z

Cool, I will pull this in.

…ainer Call get_DB_debug and make sure its cast to bool

Update

* [DEM-525] Improve raw waveform upload speed

update fork

Update QDevil branch from master

jenshnielsen added 6 commits June 7, 2017 13:55

Add some channel notebooks

2dbacc0

fix: test_channels small tweeks

cd23b07

Add support to array parameters in channels

4770840

And error clearly if you try to measure multiple multiparameters

Mock parameters add instruments to fix names in tests

4b051cb

refactor tests to reduce code duplication

6d11b5b

Revert "Add some channel notebooks"

e1c6a57

This reverts commit 2dbacc0.

jenshnielsen mentioned this pull request Jun 8, 2017

feat/channelization: Add channels to instruments microsoft/Qcodes#568

Closed

2 tasks

WilliamHPNielsen and others added 9 commits June 9, 2017 16:48

feat: Add channelised QDac driver

f05a970

Add a QDac driver using the new channelisation feature

feat: Add a set method to MultiChannelInstrumentParameter

e80de66

docs: Add a driver example for the channelised Qdac

c90ac7c

Merge pull request #2 from WilliamHPNielsen/feat/channelization_with_…

c497e65

…array Feat/channelization with array

feat: Make ChannelList take default paramclass

1793d19

Make the paramclass returned by __getattr__ be customisable.

feat: Add a channelised QDac driver

a4e4b90

Add a channelised driver for the QDac. This requires changing the base code for channels a bit to allow for custom MultiChannelInstrumentParameter subclasses.

docs: Add notebook for QDac w. channels

034fbf5

style: Remove unnecessary comment

83a32e8

Merge pull request #3 from WilliamHPNielsen/feat/channelization_with_…

eb5e700

…array Driver/QDac_channels

spauka reviewed Jun 15, 2017

View reviewed changes

jenshnielsen added 3 commits June 15, 2017 15:31

Fix: qdac_channels remove magic number

84b663b

Doc: clarify doc string

8a44cd4

Fix: rename paramclass to be more specific

24f65a2

spauka closed this Jun 16, 2017

spauka reopened this Jun 16, 2017

spauka merged commit 3145c52 into spauka:feat/channelization Jun 16, 2017

jenshnielsen deleted the feat/channelization_with_array branch June 16, 2017 05:02

spauka pushed a commit that referenced this pull request May 30, 2018

Merge pull request #2 from jenshnielsen/docs/dataset/measurement_cont…

f3d37e8

…ainer Call get_DB_debug and make sure its cast to bool

spauka pushed a commit that referenced this pull request Aug 8, 2018

Merge pull request #2 from QCoDeS/master

ce910b3

Update

spauka pushed a commit that referenced this pull request Jan 21, 2019

[DEM-525] Improve raw waveform upload speed (#2)

e231ac5

* [DEM-525] Improve raw waveform upload speed

spauka pushed a commit that referenced this pull request Feb 10, 2020

Merge pull request #2 from QCoDeS/master

ffb74da

update fork

spauka pushed a commit that referenced this pull request Mar 9, 2020

Merge pull request #2 from Anders-QDevil/master

9cffd45

Update QDevil branch from master

Comments

Conversation

jenshnielsen commented Jun 8, 2017

Uh oh!

spauka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenshnielsen commented Jun 15, 2017

Uh oh!

jenshnielsen commented Jun 15, 2017

Uh oh!

spauka commented Jun 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants