[WIP] Extension/dynamic module #1353

sohailc · 2018-10-30T06:12:28Z

This PR is still work in progress, but I would like to have comments early in the process.

astafan8 · 2018-10-30T11:13:57Z

qcodes/instrument/channel.py

+            self._parent, channel_list=self, **kwargs)
+
+        for channel in new_channels:
+            # NB: There is a bug in `extend`. TODO: Make a PR!


what is the bug about? Are you planning to make a PR with a test and a fix? That'd be great to do before landing this PR.

Ok, I will make a PR for that as well. Yes, there is a bug I discovered in the ChannelList class, but I have not come round to fixing this and making a PR. There is something wrong with the extend method in the ChannelList class, but I forgot what, exactly :-)

astafan8

I think this a good starting point. But in general my feeling is that the kwarg part of the interface is not explicit enough, and i'm looking forward to more fool-proofing measures (with tests :) ), see comments.

astafan8 · 2018-10-30T11:19:17Z

qcodes/tests/test_autoloadable_channels.py

+        )
+
+    def write_raw(self, cmd: str):
+        return self._backend.send(cmd)


write functions don't return, right?)

Yes, that is true. I will fix this.

astafan8 · 2018-10-30T11:19:21Z

qcodes/tests/test_autoloadable_channels.py

+
+    def _create(self)->None:
+        """Create the channel on the instrument"""
+        self.parent.write(f":INST:CHN:ADD {self._channel}")


shouldn't self.root_instrument.write be used instead of parent? note that the docstring of _create in the class suggests that.

Yes, this should be root_instrument. Good catch!

qcodes/instrument/channel.py

astafan8 · 2018-10-30T11:35:35Z

qcodes/instrument/channel.py

+        raise NotImplementedError("Please subclass")
+
+    @classmethod
+    def get_new_instance_kwargs(cls, parent: Instrument, **kwargs)->dict:


when I look at the usage of this method, I struggle to understand something. Do you mean that through this method a channel class obtains also the values of kwargs that it needs for its __init__? From the test below, I deduct that it is expected the channel will need to basically refer to the parent instrument to get some important info (like the list of existing channels, so that the new channel can get a unique id that does not clash with the existing channels). What if for some instruments it's not the case? I'd propose to have parent as an optional kwarg.

Sure, we can make parent an optional keyword argument. Some instruments indeed do not need the parent instrument, while other do.

qcodes/instrument/channel.py

astafan8 · 2018-10-30T12:10:24Z

qcodes/tests/test_autoloadable_channels.py

+        self.add_submodule("channels", channels)
+
+
+def test_sanity():


thanks for the test! but an example notebook with at least (for now) bare intentions of this infra, would be appreciated. This will also help to (if necessary) improve the design, strip down excessive code, and perhaps even add something. (yes, yes, i know that everything is expressed in the description of the linked issue, but still :) )

I honestly think that adding a driver which uses this infrastructure will be illuminating, perhaps even more so than a notebook as it will show the complexities involved with a real instrument. I will make the PNA driver use the infrastructure which I think will be helpful.

qcodes/instrument/channel.py

qcodes/tests/test_autoloadable_channels.py

Co-Authored-By: sohailc <schatoor@gmail.com>

2) Make the parent argument of get_new_instance_kwargs optional 3) create should first assert that the channel does not exist 4) delete should first assert that the channel exists 5) delete should first check that self is in channels list 6) In the test channel class, use parent.root_instrument 7) In the test channel class, write does not return 8) In the test channel class, check for the specific error message in raise assertions. 9) In the test channel class, use fixtures 10) Make a new_instance method 11) More clear doc string in _get_new)instance_kwargs

…ion/dynamic_module

astafan8 · 2018-10-31T09:14:36Z

qcodes/tests/test_autoloadable_channels.py

-    with pytest.raises(RuntimeError):
+    # Once we remove the channel we should no longer be able to talk to the instrument
+    new_channel.remove()
+    with pytest.raises(RuntimeError) as e2:


you can use pytest.raises(Exception, match="Object does not exist (anymore) on the instrument") instead of working with e2 explicitly.

Oh I did not know that! Thanks!

Actually, Mikhail, we should message the kwarg message instead of match

…the channel_list argument.

…ion/dynamic_module

… into extension/dynamic_module # Conflicts: # qcodes/instrument/channel.py # qcodes/tests/test_autoloadable_channels.py

…R. Temporarily housed here to verify effectiveness

…into extension/dynamic_module

qcodes/instrument/channel.py

codecov · 2018-11-01T19:32:45Z

Codecov Report

Merging #1353 into master will increase coverage by 0.06%.
The diff coverage is 82.6%.

@@            Coverage Diff             @@
##           master    #1353      +/-   ##
==========================================
+ Coverage   73.22%   73.29%   +0.06%     
==========================================
  Files          79       79              
  Lines        9188     9256      +68     
==========================================
+ Hits         6728     6784      +56     
- Misses       2460     2472      +12

sohailc · 2018-11-01T20:28:23Z

@astafan8 Can you approve this PR if you have no further objections? The tests and CI seem to be passing.
@Dominik-Vogel Can you have a look at this PR as well?

astafan8 · 2018-11-07T11:24:58Z

@sohailc If you say that the PR is ready, please remove WIP from the title, and update the description :)

I'm reviewing now...

astafan8

It looks good! It's almost ready modulo some typos and clarifications (see my comments).

qcodes/instrument/channel.py

qcodes/tests/test_autoloadable_channels.py

…ion/dynamic_module

Give more useful hints when `_get_new_instance_kwargs` raises an exception.

…uld state that the newly created instance is returned.

astafan8

Good! I think this is ready to go. What do you think, @QCoDeS/core ?

Merge: 4dd73ba 4a4825d Author: sohail chatoor <schatoor@gmail.com> Merge pull request #1353 from sohailc/extension/dynamic_module

sohailc added 6 commits October 27, 2018 13:13

Added auto load channels

47138b0

Added test module for autoloadable channels

d6d7049

added sanity test

0ff6d12

added channel list test

bce85ef

Added more complete test

dc7a47e

test that after deletion, channel is not in a channels list

99b1522

sohailc added the enhancement label Oct 30, 2018

sohailc self-assigned this Oct 30, 2018

sohailc requested review from astafan8 and Dominik-Vogel October 30, 2018 06:12

Merge branch 'master' into extension/dynamic_module

37395bd

sohailc mentioned this pull request Oct 30, 2018

Make it possible to dynamically create/discover/delete sub-modules on instruments #1342

Closed

astafan8 changed the title ~~Extension/dynamic module~~ [WIP] Extension/dynamic module Oct 30, 2018

astafan8 reviewed Oct 30, 2018

View reviewed changes

astafan8 requested changes Oct 30, 2018

View reviewed changes

astafan8 and others added 4 commits October 30, 2018 06:40

Good! I like it

62a1a1b

Co-Authored-By: sohailc <schatoor@gmail.com>

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

0a47402

…ion/dynamic_module

1) Added tests for keyword arguments.

8bf09fc

astafan8 reviewed Oct 31, 2018

View reviewed changes

sohailc added 10 commits October 31, 2018 09:32

Merge branch 'master' into extension/dynamic_module

f3cd79a

improved formatting

53da990

bug fix: when loading channels from instrument, make sure to pass on …

ae583f0

…the channel_list argument.

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

8378df9

…ion/dynamic_module

Merge remote-tracking branch 'qcodes-sohail/extension/dynamic_module'…

9099600

… into extension/dynamic_module # Conflicts: # qcodes/instrument/channel.py # qcodes/tests/test_autoloadable_channels.py

improved formatting

84b7b2d

Ignore mypy in AutoLoadableChannelList

f88d84e

type should not be in quotation marks

7bad06a

fixed channel list extend: This needs to be replaced to a different P…

9d5d93e

…R. Temporarily housed here to verify effectiveness

removed fix as it is in another PR

4935cc2

Merge branch 'fix/channel_extend' of https://github.com/sohailc/Qcodes …

ae4a823

…into extension/dynamic_module

sohailc commented Nov 1, 2018

View reviewed changes

qcodes/instrument/channel.py Show resolved Hide resolved

Use message kwarg in pytest.raises

01f7b76

sohailc and others added 2 commits November 2, 2018 08:52

Merge branch 'master' into extension/dynamic_module

151c37a

Merge branch 'master' into extension/dynamic_module

0b233b3

astafan8 requested changes Nov 7, 2018

View reviewed changes

sohailc added 7 commits November 7, 2018 09:24

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

beb6541

…ion/dynamic_module

Processes PR comments.

f42262c

Give more useful hints when `_get_new_instance_kwargs` raises an exception.

Typo in docstring

342efa8

the add method docstring in the class AutoLoadableChannelList sho…

e3058fd

…uld state that the newly created instance is returned.

newline as suggested by astafan8

91afce5

fixed 'unused import' nonsense from CI

94f257e

Merge branch 'master' into extension/dynamic_module

29b7a0c

astafan8 approved these changes Nov 12, 2018

View reviewed changes

Merge branch 'master' into extension/dynamic_module

4a4825d

sohailc merged commit 9c7b9a0 into microsoft:master Nov 12, 2018

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

Generated gh-pages for commit 9c7b9a0

a0b714b

Merge: 4dd73ba 4a4825d Author: sohail chatoor <schatoor@gmail.com> Merge pull request #1353 from sohailc/extension/dynamic_module

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Extension/dynamic module #1353

[WIP] Extension/dynamic module #1353

sohailc commented Oct 30, 2018

astafan8 Oct 30, 2018

sohailc Oct 30, 2018

astafan8 left a comment

astafan8 Oct 30, 2018

sohailc Oct 30, 2018

astafan8 Oct 30, 2018

sohailc Oct 30, 2018

astafan8 Oct 30, 2018

sohailc Oct 30, 2018

astafan8 Oct 30, 2018

sohailc Oct 30, 2018

astafan8 Oct 31, 2018

sohailc Nov 1, 2018

sohailc Nov 1, 2018

codecov bot commented Nov 1, 2018 •

edited

sohailc commented Nov 1, 2018

astafan8 commented Nov 7, 2018

astafan8 left a comment

astafan8 left a comment

[WIP] Extension/dynamic module #1353

[WIP] Extension/dynamic module #1353

Conversation

sohailc commented Oct 30, 2018

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

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

codecov bot commented Nov 1, 2018 • edited

Codecov Report

sohailc commented Nov 1, 2018

astafan8 commented Nov 7, 2018

astafan8 left a comment

Choose a reason for hiding this comment

astafan8 left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 1, 2018 •

edited