Feat/channelization further work #641

jenshnielsen · 2017-06-16T09:31:52Z

A few additional changes on top of the channelization

Refactor instrument and instrumentchannel to inherit from instrumentbase, to avoid removing methods from channel
Convert the channels within a channel list to a named tuple, allowing them to be acesed as inst.channels.channelname
~~Add support for allowing channels to be one indexed~~
make sure that channels are included in print_readable_snapshot

@spauka @WilliamHPNielsen

WilliamHPNielsen

Review of channel.py

Apart from these modest changes:

PEP8! Half the file has too long lines
Some comments are now obsolete

WilliamHPNielsen · 2017-06-19T12:00:30Z

qcodes/instrument/channel.py

@@ -167,9 +140,10 @@ class ChannelList(Metadatable):

    """

-    def __init__(self, parent, name, chan_type: type, chan_list=None,
+    def __init__(self, parent: Instrument, name, chan_type: type, chan_list=None,


Either we use type annotations or we don't (and I guess we are moving towards using them), but we should not mix. Also, the __init__ function raises errors if chan_type and multichan_paramclass are not something which is more specific than type.

Why not, it's gradual typing after all ;) That aside it's easy to add them but the main benefit comes from annotating the chan_type because the linter has trouble figuring out that we are passing a Class and not an instance of a Class but sure it's easy to add the rest.

I am not sure that there is a better way of annotating that this is a Class not an instance of the Class. type(multichan_paramclass) is not what you are looking for at least.

WilliamHPNielsen · 2017-06-19T12:02:28Z

qcodes/instrument/channel.py

+                               oneindexed=self._oneindexed)
+        elif self._oneindexed:
+            if i < 1:
+                raise IndexError("1 indexed channel lists only support positive indexes")


Same as above.

WilliamHPNielsen · 2017-06-19T12:03:39Z

qcodes/instrument/channel.py

@@ -206,9 +181,18 @@ def __getitem__(self, i):
            i (int/slice): Either a single channel index or a slice of channels to get
        """
        if isinstance(i, slice):
+            if self._oneindexed:
+                if i.start < 1 or i.stop < 1:
+                    raise IndexError("1 indexed channel lists only support positive indexes")


Merriam-Webster allows for both forms, but I prefer "indices" over "indexes".

WilliamHPNielsen · 2017-06-19T12:05:18Z

qcodes/instrument/channel.py

@@ -279,7 +264,7 @@ def index(self, obj):
        Args:
            obj(chan_type): The object to find in the channel list.
        """
-        return self._channels.index(obj)
+        return self._channels.index(obj) + self._oneindexed


Maybe it's just me, but I would find self._channels.index(obj) + int(self._oneindexed) to be more obvious.

WilliamHPNielsen · 2017-06-19T12:05:40Z

qcodes/instrument/channel.py

@@ -296,6 +281,11 @@ def insert(self, index, obj):
            raise TypeError("All items in a channel list must be of the same type."
                            " Adding {} to a list of {}.".format(type(obj).__name__,
                                                                 self._chan_type.__name__))
+        if self._oneindexed:
+            if index < 1:
+                raise IndexError("1 based channel lists only support positive indexes")


Same as above.

WilliamHPNielsen · 2017-06-19T12:25:57Z

qcodes/instrument/base.py

+        self.name = str(name)
+        self.parameters = {}
+        self.functions = {}
+        self.submodules = {}


Why does the InstrumentBase have submodules? Does it need them? And if so, should it not be in the docstring and there be a add_submodule method?

I guess we still need the submodules mentioned in the docstring.

WilliamHPNielsen · 2017-06-19T12:26:55Z

qcodes/instrument/base.py

+
+        print(self.name + ':')
+        print('{0:<{1}}'.format('\tparameter ', par_field_len) + 'value')
+        print('-'*80)


80 should probably be max_chars.

WilliamHPNielsen · 2017-06-19T12:27:33Z

qcodes/instrument/base.py

+                submodule.print_readable_snapshot(update, max_chars)
+
+    #
+    # shortcuts to parameters & setters & getters                            #


One space too many 😺

WilliamHPNielsen · 2017-06-19T14:01:38Z

qcodes/instrument/base.py

+
+        print(self.name + ':')
+        print('{0:<{1}}'.format('\tparameter ', par_field_len) + 'value')
+        print('-'*80)


This 80 character long line should probably be max_chars long.

jenshnielsen · 2017-06-19T15:32:05Z

Should check how this works with the QCodes Monitor

WilliamHPNielsen

Re the changes: Soooo pretty 😃
Re monitor: Agreed.

spauka · 2017-06-20T06:57:16Z

Just quickly, apologies for not responding earlier, is it worth having a discussion of whether allowing 1-indexing is a good idea? It created a syntax that is counter to what a python user would expect.

…

On Tue., 20 Jun. 2017, 8:55 am William H.P. Nielsen, < ***@***.***> wrote: ***@***.**** approved this pull request. Soooo pretty 😃 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#641 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAmyvA7u4uvf2Wrfshf2uQcPKyRI7Jhbks5sF2zZgaJpZM4N8PKx> .

jenshnielsen · 2017-06-20T07:39:10Z

Im not sure, I don't particularly like it either but I fear that users will be very unhappy with a situation where chan[0] maps to channel 1 on an instrument

spauka · 2017-06-20T08:06:05Z

Perhaps there can be a logical separation between channel names and indexing though. While naming ports by their names as (like `vna.Port1..4`) makes sense since this is explicitly instrument specific, while indexing them using list notation, this creates a situation where similar instruments could behave in two different ways. For example, the Harvard dac naturally indexes from zero, while the QDac does not. Code written for one over the other now does not port over cleanly! There is now no longer a possibility to read code without knowing whether certain instruments index by 1 or 0. Indexing by 0 is sufficiently universal in coding that even if a user is not a coder, they will encounter such syntax everywhere so I don't think it is an unreasonable ask for users here.

WilliamHPNielsen · 2017-06-20T08:30:10Z

Hmm... It's bit hard to be strongly in favour of either.

I don't see porting of code from one driver to the other ever happening easily, so I am not very concerned about that aspect of things. Instruments are in my experience always in a world of their own, and two boxes never quite behave the same way.

Having 1-indexed python lists are not nice, that is for sure. They are not too uncommon in the scientific world, though, with both Matlab/Octave and Mathematica indexing from 1, but I agree that python should be kept with one convention.

My main concern (and Jens' too, I suspect) is that users will immediately complain and/or make their own patching of the driver too be 1-indexed. And just before doing so, they will accidentally fry a sample by setting DAC channel 23 to 10 Volts using dac.channels[22]. Is there a good way to avoid that?

nulinspiratie · 2017-06-20T08:34:13Z

Would a dict instead of a list solve this? In this case, there doesn't have to be a separation between name and index, keys could even be numbers

…

On Tue, Jun 20, 2017, 6:30 PM William H.P. Nielsen ***@***.***> wrote: Hmm... It's bit hard to be strongly in favour of either. I don't see porting of code from one driver to the other ever happening easily, so I am not very concerned about that aspect of things. Instruments are in my experience always in a world of their own, and two boxes never quite behave the same way. Having 1-indexed python lists are not nice, that is for sure. They are not too uncommon in the scientific world, though, with both Matlab/Octave and Mathematica indexing from 1, but I agree that python should be kept with one convention. My main concern (and Jens' too, I suspect) is that users will immediately complain and/or make their own patching of the driver too be 1-indexed. And just before doing so, they will accidentally fry a sample by setting DAC channel 23 to 10 Volts using dac.channels[22]. Is there a good way to avoid that? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#641 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGgLHQjOGoB_sUtw4VMtoRrmPfha46rYks5sF4MTgaJpZM4N8PKx> .

spauka · 2017-06-20T08:44:25Z

@nulinspiratie there is already a method to address channels by name (i.e. dac.c0 or now dac.channels.c0 both work). If the indexing syntax changes to a dict, i.e. dac.channels['c0'] then slicing no longer makes sense (no way to do dac.channels[c2:c4])

The fact is that adding 1-indexing creates the opposite problem for python users. Someone coming in could very easily set c[20] thinking it addresses channel 21!

nulinspiratie · 2017-06-20T11:37:25Z

@spauka Ah right, but why not override slicing in __getitem__? As I see it, you ideally want a single way to represent your channel, by its name. Accessing multiple channels through dac.channels['c2':'c4'] would feel more intuitive than first converting it to indices. One case where this makes sense is instruments that use channel letters instead of numbers. If you need to remember both a letter and a number for a channel, it can quickly get confusing.
I was thinking of something along the lines of

class ChannelList:
    def __getitem__(self, sliced):
        print(f'getting channels {sliced.start} to {sliced.stop}')

channel_list = ChannelList()
channel_list['chA':'chC']

which outputs: getting channels chA to chC

jenshnielsen · 2017-06-21T14:21:57Z

Agreed with @spauka and @WilliamHPNielsen to go for the simplest solution now and only support regular python lists/tuples if the need arises we can come up with a more clever solution

jenshnielsen · 2017-06-21T14:36:40Z

With regards to the monitor, Channel parameters are supported as well as regular parameters.

Namely you cannot pass a MultiChannelParameter to the monitor but that is no different from a regular MultiParameter. Each channel is rendered as a new instrument which is suboptimal but probably ok for now

spauka · 2017-06-22T08:15:24Z

self._submodules and add_submodule should also be part of instrument base, as there is the possibility for nested channel lists, for example in the driver for the Harvard DAC, the list of slots also contains a list of channels, meaning the add_submodule function is required in InstrumentChannel.

That way you can access the names on the channellist via names as soon as they are locked

many instruments have hardware channels that are indexed and start at 1

jenshnielsen · 2017-06-22T11:09:40Z

rebased and pushed submodules up to instrumentbase I think this is ready to merge

Make it possible to get channels by name even when not locked

jenshnielsen · 2017-06-27T09:08:00Z

I changed it around a bit to make it possible to index channels by name without locking the channels, I think this is ready to be merged

WilliamHPNielsen · 2017-06-27T13:03:13Z

If we mention submodules in the docstring for InstrumentBase, I am happy.

jenshnielsen · 2017-06-27T13:55:44Z

@WilliamHPNielsen done

WilliamHPNielsen

Beautiful!

Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com> Feat/channelization further work (#641)

* fix: refactor Instrument to avoid removing methods from channels * Include channels in print_readable_snaphoot * Fix: make locked channels a named tuple That way you can access the names on the channellist via names as soon as they are locked * Fix: channels add support for indexing from 1 many instruments have hardware channels that are indexed and start at 1 * Fix linting errors in channel * fix typo * Move print readable snapshot to instrument class where it belongs * Fix: more channel annotation * Improve error message * pprint limit line lenght of header to max char * pep8 line lenght * Fix: channel remove support for oneindexed channels * improve type annotation * Optional -> Union * add channels to api docs * fix: make submodules nested * add submodule attributes to base * Replace namedtuple by dict Make it possible to get channels by name even when not locked * Add submodule to docstring

WilliamHPNielsen suggested changes Jun 19, 2017

View reviewed changes

WilliamHPNielsen reviewed Jun 19, 2017

View reviewed changes

WilliamHPNielsen approved these changes Jun 20, 2017

View reviewed changes

jenshnielsen added 15 commits June 22, 2017 13:01

fix: refactor Instrument to avoid removing methods from channels

7045411

Include channels in print_readable_snaphoot

4b93576

Fix: make locked channels a named tuple

31cc34c

That way you can access the names on the channellist via names as soon as they are locked

Fix: channels add support for indexing from 1

8734ab4

many instruments have hardware channels that are indexed and start at 1

Fix linting errors in channel

75b3fa6

fix typo

4b1b5d0

Move print readable snapshot to instrument class where it belongs

0610ee5

Fix: more channel annotation

de6d1ed

Improve error message

dd471b3

pprint limit line lenght of header to max char

c3c7d5b

pep8 line lenght

032144b

Fix: channel remove support for oneindexed channels

f0b6f44

improve type annotation

2bdf316

Optional -> Union

a88436f

add channels to api docs

6447561

jenshnielsen added 2 commits June 22, 2017 13:05

fix: make submodules nested

b38ad6e

add submodule attributes to base

cc56d6a

jenshnielsen force-pushed the feat/channelization_furter_work branch from 354b3cd to cc56d6a Compare June 22, 2017 11:08

Replace namedtuple by dict

14ea564

Make it possible to get channels by name even when not locked

Merge branch 'master' into feat/channelization_furter_work

fd60be2

Add submodule to docstring

91c0c4c

WilliamHPNielsen approved these changes Jun 27, 2017

View reviewed changes

Merge branch 'master' into feat/channelization_furter_work

9699bd9

jenshnielsen merged commit 8dfa2ab into microsoft:master Jul 11, 2017

giulioungaretti pushed a commit that referenced this pull request Jul 11, 2017

Generated gh-pages for commit 8dfa2ab

4bab697

Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com> Feat/channelization further work (#641)

AdriaanRol mentioned this pull request Sep 30, 2017

Node-tree based parameter structure in instruments #773

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/channelization further work #641

Feat/channelization further work #641

jenshnielsen commented Jun 16, 2017 •

edited

WilliamHPNielsen left a comment •

edited

WilliamHPNielsen Jun 19, 2017

jenshnielsen Jun 19, 2017

WilliamHPNielsen Jun 19, 2017

WilliamHPNielsen Jun 19, 2017

WilliamHPNielsen Jun 19, 2017

WilliamHPNielsen Jun 19, 2017

WilliamHPNielsen Jun 19, 2017

WilliamHPNielsen Jun 27, 2017

WilliamHPNielsen Jun 19, 2017

WilliamHPNielsen Jun 19, 2017

WilliamHPNielsen Jun 19, 2017 •

edited

jenshnielsen commented Jun 19, 2017

WilliamHPNielsen left a comment •

edited

spauka commented Jun 20, 2017 via email

jenshnielsen commented Jun 20, 2017

spauka commented Jun 20, 2017 via email •

edited

WilliamHPNielsen commented Jun 20, 2017

nulinspiratie commented Jun 20, 2017 via email

spauka commented Jun 20, 2017

nulinspiratie commented Jun 20, 2017

jenshnielsen commented Jun 21, 2017

jenshnielsen commented Jun 21, 2017

spauka commented Jun 22, 2017 •

edited by WilliamHPNielsen

jenshnielsen commented Jun 22, 2017

jenshnielsen commented Jun 27, 2017

WilliamHPNielsen commented Jun 27, 2017

jenshnielsen commented Jun 27, 2017

WilliamHPNielsen left a comment

Feat/channelization further work #641

Feat/channelization further work #641

Conversation

jenshnielsen commented Jun 16, 2017 • edited

WilliamHPNielsen left a comment • edited

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

WilliamHPNielsen Jun 19, 2017 • edited

Choose a reason for hiding this comment

jenshnielsen commented Jun 19, 2017

WilliamHPNielsen left a comment • edited

Choose a reason for hiding this comment

spauka commented Jun 20, 2017 via email

jenshnielsen commented Jun 20, 2017

spauka commented Jun 20, 2017 via email • edited

WilliamHPNielsen commented Jun 20, 2017

nulinspiratie commented Jun 20, 2017 via email

spauka commented Jun 20, 2017

nulinspiratie commented Jun 20, 2017

jenshnielsen commented Jun 21, 2017

jenshnielsen commented Jun 21, 2017

spauka commented Jun 22, 2017 • edited by WilliamHPNielsen

jenshnielsen commented Jun 22, 2017

jenshnielsen commented Jun 27, 2017

WilliamHPNielsen commented Jun 27, 2017

jenshnielsen commented Jun 27, 2017

WilliamHPNielsen left a comment

Choose a reason for hiding this comment

jenshnielsen commented Jun 16, 2017 •

edited

WilliamHPNielsen left a comment •

edited

WilliamHPNielsen Jun 19, 2017 •

edited

WilliamHPNielsen left a comment •

edited

spauka commented Jun 20, 2017 via email •

edited

spauka commented Jun 22, 2017 •

edited by WilliamHPNielsen