New AMI430 driver by sohailc · Pull Request #700 · microsoft/Qcodes

sohail chatoor (sohailc) · 2017-08-10T14:52:42Z

This should fix issue 648.

I have added some tests to see if this is the case.

I have also added an environment.yml file so that to install the enviromnent to develop and run qcodes, we can just do (from the command line):

git clone
cd qcodes
conda env create

2) added a mock IP module to mock IP instruments 3) added a debug module to debug the AMI430 instrument

…rdinates. Work in progress

…cylindrical coodinates. By keeping a prepresnetation of the vector in all three systems, there is less risk of loss of information so that e.g. the field strength can be ramped up given previous supplied phi and theta. The spherical and cylindrical tests seem to work.

2) Made proper tests

…uments 2) Made test to verify that the correct error is raised if we intentionally exceed the field limits

… Tests to verify this are also included.

…e mocking mechanism

…rdinates. Added tests to verify if we can get spherical coordinates. Also, when getting single field values, return a single float instead of an array of length one

2) Added test to verify that the correct warnings are raised if we change the default maximum ramp rate 3) Added test to verify that an error is raised if the ramp rate larger then the maximum

… driver

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-25T07:40:40Z

+  <component name="VcsDirectoryMappings">
+    <mapping directory="$PROJECT_DIR$" vcs="Git" />
+  </component>
+</project>


I don't think this should go into version control?

probably add it to .gitignore

For some reason, I cannot remove this file from the PR. Any idea's?

Ok Lets leave it for now, If it contains local specific information we can remove it later

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-25T08:24:55Z

+channels: 
+ - anaconda
+ - conda-forge
+ - dsdale24


I think the channels should just be:

- defaults - conda-forge

The dsdate24 is not used and the preferred name for the default is default. With the setup above I ended up with all packages looking like they are outdated because default has higher priority than anaconda

I will make the changes as you suggest and see if I can create a working environment.

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-25T08:57:07Z

+
+    def __init__(self, name, testing=False, **kwargs):
        self.name = str(name)
+        self.testing = testing


This should probably be a private attribute _testing

yeah good point

We could also make it a property that cannot be set and get rid of the is_testing method below. Whichever way you prefer but there should only be one public attribute/method

Jens Hedegaard Nielsen (jenshnielsen)

Looks good to me. Left some more inline comments

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-25T09:04:07Z

+    def get_mock_messages(self):
+        """
+        For testing purposes we might want to get log messages from the mocker.
+        :return: mocker_messages: list, str


We are generally using google style with sphinx napoleon http://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html rather than native formatting of doc strings

I will adjust the doc strings

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-25T09:24:28Z

+            "functions": {name: func.snapshot(update=update) for name, func in self.functions.items()},
+            "submodules": {name: subm.snapshot(update=update) for name, subm in self.submodules.items()},
+            "__class__": full_class(self)
+        }


We have not been enforcing it but we have been trying to move towards pep8 and <79 char per line

I do not exactly understand why this is giving me pep8 issues. My pycharm editor also complains about this line, but I do not understand why.

The line is longer than 79 characters now? no. The wrapping was to make it shorter

It is complaining that the dictionary can be re-written as a dict literal. So literal:

some_dict = {"A": 1, "B": 2}

vs.

dict(A=1, B=1).

But as far as I can see the dictionary is literal, so I do not understand the complaint.

This link has some explaination: link, but I do not understand how this applies here. Anyway, I do not think this issue is very important and I propose to ignore it.

Agreed that seems wrong to me. I think its safe to ignore that

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-30T08:06:27Z

+from datetime import datetime
+
+
+class MockAMI430(object):


No need to inherit from object. We only support python3

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-30T08:09:27Z

+        :param msg_str: string, the message string the mock instrument gets through the network socket.
+        :param key: string, one of the keys in self.handlers
+        :return: (match, args), (bool, string), if the key and the msg_str match, then match = True. If any arguments
+                                                are present in the message string these will be passed along.


Google style

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-30T08:37:20Z

@@ -0,0 +1,463 @@
+{


This file should go in the drivers example folder (1 below this one)

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-30T08:38:39Z

+   "outputs": [],
+   "source": [
+    "import sys \n",
+    "sys.path.append(r\"C:\\Users\\a-sochat\\development\\Qcodes_QUTech\")\n",


Why is this needed. We should aim at having all notebooks something that users can run if they have the right hardware

This is needed because the notebook is not in the root folder of the qcodes module. If the qcodes module is not in a location where python can find it, we need to specifically add the module path to the system path. If the user running this notebook has qcodes installed in a standard location then this is not necessary. I will remove the line from the notebook

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-30T09:00:26Z

+   "cell_type": "markdown",
+   "metadata": {},
+   "source": [
+    "![title](AMI430_test_cartesian.jpg)\n",


Is this image added to the repository

I think I should remove the picture all togehter. I don't think we should have large images in the repo. A comment that we have manually checked that the instrument displays show that the correct currents are reached should suffice.

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-30T09:01:40Z

+        "theta": np.random.uniform(0, 180),
+        "phi": np.random.uniform(0, 360),
+        "rho": np.random.uniform(0, 1)
+    }[coordinate_name]


I would use hypothesis https://hypothesis.readthedocs.io/en/latest/ in any test that relies on random data

I do not know that module but I will familiarize myself and use it here.

It not needed, only a suggestion because I think it's cool and we are already using it. Feel free to leave as it is

wow thats a useful library. Good to know!

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-30T12:11:24Z

+        """
+        :param msg: string, a message received through the socket communication layer
+        :return: string or None,    If the type of message requests a value (a get message) then this value is returned
+                                    by this function. A set message will return a None value.


google style

Jens Hedegaard Nielsen (jenshnielsen) · 2017-08-30T12:13:53Z

+         1) There are contradictory inputs (e.g. x=3, y=4, z=0 and rho=6)
+        """
+
+        for _ in range(1):


Why is this for loop needed?

I would remove it and replace the breaks with return. That should be equivalent?

I think a break statement only works in loops.Your suggestions would indeed be equivalent. Ideally we want to have a single exit point in fuctions, however my solution is not ideal either. I will replace the breaks by return.

Btw... the situation would be a little trickier if after the loop we had other code. We could not simply return as the rest of the code would not be reachable. A better solution has been implemented in the code. See the updated PR shortly

…pep8 conventions. Will do that tomorrow

Author: sohail chatoor <schatoor@gmail.com> New AMI430 driver (#700)

sohail chatoor and others added 16 commits August 7, 2017 16:22

Added environment.yml file

fbcdd2d

Added requirements to environment.yml

3757cdc

Added requirements to environment.yml

d4c8ed1

Works under linux now

1397535

1) added american instrument driver from the main qcodes repo

2888633

2) added a mock IP module to mock IP instruments 3) added a debug module to debug the AMI430 instrument

the mock IP instrument now remembers the set point

0927761

added test for spherical coordinates. Making test for cylindrical coo…

478d8ec

…rdinates. Work in progress

additional work

ef19eff

resolved merge conflict

c0095af

1) The driver is adhering to the qcodes specs now

c591196

2) Made proper tests

1) The driver is adhering to the qcodes specs now

eb2ab7d

2) Made proper tests

1) Made tests to ensure that the set points correctly reach the instr…

9f321c3

…uments 2) Made test to verify that the correct error is raised if we intentionally exceed the field limits

Added a test to verify that ramp downs are performed first

cc6073d

Merge branch 'master' of git://github.com/qcodes/qcodes

4f0fd7b

Added the ability to define more complex regions for safe set points.…

5838d0d

… Tests to verify this are also included.

sohail chatoor (sohailc) mentioned this pull request Aug 16, 2017

Driver test. #475

Open

sohail chatoor added 11 commits August 17, 2017 14:29

Re-wrote the instrument mocker so we have a cleaner design.

1ef9f8e

Merge branch 'master' of git://github.com/qcodes/qcodes

f245439

changing how mocking works

dd0aee0

Mocking no longer requires sockets and threading. Major rewrite of th…

ccabe4e

…e mocking mechanism

Mocking no longer requires sockets and threading. Major rewrite of th…

d69583c

…e mocking mechanism

Merge branch 'master' of git://github.com/qcodes/qcodes

590925e

further clean up of the mocking mechanism.

14f683e

Solved a bug which raised a value error when asking for spherical coo…

013c38b

…rdinates. Added tests to verify if we can get spherical coordinates. Also, when getting single field values, return a single float instead of an array of length one

Merge branch 'master' of git://github.com/qcodes/qcodes

b2f7203

1) Added tests for the field vector

f52052d

2) Added test to verify that the correct warnings are raised if we change the default maximum ramp rate 3) Added test to verify that an error is raised if the ramp rate larger then the maximum

Added a notebook showing the results of the testing of the new AMI430…

6423afa

… driver

Jens Hedegaard Nielsen (jenshnielsen) reviewed Aug 25, 2017

View reviewed changes

Jens Hedegaard Nielsen (jenshnielsen) requested changes Aug 30, 2017

View reviewed changes

sohail chatoor added 3 commits August 30, 2017 18:46

* Processed Jens comments, except changing the doc string format and …

0809e6d

…pep8 conventions. Will do that tomorrow

Merge branch 'master' of git://github.com/qcodes/qcodes

1c93c8e

Processed the rest of Jens comments

77a55e8

Jens Hedegaard Nielsen (jenshnielsen) approved these changes Sep 4, 2017

View reviewed changes

Jens Hedegaard Nielsen (jenshnielsen) merged commit 5acbeeb into microsoft:master Sep 4, 2017

Giulio Ungaretti (giulioungaretti) pushed a commit that referenced this pull request Sep 4, 2017

Generated gh-pages for commit 5acbeeb

d280860

Author: sohail chatoor <schatoor@gmail.com> New AMI430 driver (#700)

Giulio Ungaretti (giulioungaretti) pushed a commit that referenced this pull request Sep 4, 2017

Generated gh-pages for commit 5acbeeb

790f9f1

Author: sohail chatoor <schatoor@gmail.com> New AMI430 driver (#700)

Conversation

sohail chatoor (sohailc) commented Aug 10, 2017

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!

Jens Hedegaard Nielsen (jenshnielsen) 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!

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!

Choose a reason for hiding this comment

Uh oh!