Various IPS120 driver improvements #1402

spxtr · 2018-11-28T23:16:47Z

First I make it support both GPIB and serial. Next, fix some bugs in the driver. I haven't yet tested this on my own machine, will do so soon.

@jenshnielsen

codecov · 2018-11-29T01:53:03Z

Codecov Report

Merging #1402 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1402      +/-   ##
==========================================
- Coverage   73.88%   73.86%   -0.03%     
==========================================
  Files          92       92              
  Lines       10420    10411       -9     
==========================================
- Hits         7699     7690       -9     
  Misses       2721     2721

astafan8

This looks good.

But I am tempting to ask you to go through the whole driver, and perform the same refactoring for other parameters: extracting the number<->string dictionaries out of the methods into class attributes as you did for _STATUS_MODE2 and _STATUS_SWITCH_HEATER :)

jenshnielsen

Agreed with @astafan8, It would be great if you have the time to perform all these conversions. I suggested inline to use self.log I realize that the driver does not do that in general. We are in the middle of converting the drivers to that pattern to make it simpler to get log messages from a specific driver only. It would be great if you are willing to do this conversion over the driver.
Otherwise this looks great

qcodes/instrument_drivers/oxford/IPS120.py

jenshnielsen · 2018-12-04T12:12:10Z

qcodes/instrument_drivers/oxford/IPS120.py

-                    mode,
-                    "Unknown"))
+        if mode in status:
+            log.info('Setting remote control status to %s' % status[mode])


I think this should be self.log See https://qcodes.github.io/Qcodes/examples/Creating%20Instrument%20Drivers.html#Logging

spxtr · 2018-12-11T03:48:24Z

OK, I made the driver use self.log instead of log, pulled status dictionaries out, and tidied some random things up. Please let me know how else I can improve it.

I have tested something very similar to this on a real machine, but not this exact change, so please let me test it again before merging.

qcodes/instrument_drivers/oxford/IPS120.py

jenshnielsen · 2018-12-11T08:37:42Z

It looks like there are a few small syntax errors. Other than than I think this is ready to merge once you confirm that it has been tested on a real device

Also bring the driver up to date in terms of style.

jenshnielsen · 2019-01-16T14:04:02Z

@spxtr Did you ever test this on a real device

spxtr · 2019-01-16T23:17:57Z

We've warmed up the system for maintenance, once we cool it back down I'll test this. Should be in a week or two.

astafan8

I left some comments, it would be very nice to finalize the great refactoring that you did! :)

But mostly the "request changes" part is for us to know that we can merge only after the tests on actual instruments are done.

qcodes/instrument_drivers/oxford/IPS120.py

astafan8 · 2019-01-17T10:14:30Z

qcodes/instrument_drivers/oxford/IPS120.py

+            0: "Amps, Magnet sweep: fast",
+            1: "Tesla, Magnet sweep: fast",
+            4: "Amps, Magnet sweep: slow",
+            5: "Tesla, Magnet sweep: slow"}


isn't this dict missing 8 and 9? see docstring here https://github.com/QCoDeS/Qcodes/pull/1402/files#diff-05c52e149e6fdc1a4a23377634638f2cR862

qcodes/instrument_drivers/oxford/IPS120.py

astafan8

Thanks for the changes! The commit message says that the driver has been tested, right? Could you confirm it, and then I'll merge? Thanks :)

I did leave some minor comments, but they are related to my understanding of the driver, not to the PR.

qcodes/instrument_drivers/oxford/IPS120.py

spxtr · 2019-02-17T06:46:06Z

Yes, I tested this on a real machine. At least the basic functions like manipulating the switch heater, running to fields, and entering/leaving persistent mode worked.

codecov · 2019-02-18T09:02:01Z

Codecov Report

Merging #1402 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1402   +/-   ##
=======================================
  Coverage   73.88%   73.88%           
=======================================
  Files          92       92           
  Lines       10420    10420           
=======================================
  Hits         7699     7699           
  Misses       2721     2721

Merge: 18054f5 36f4d99 Author: Mikhail Astafev <astafan8@gmail.com> Merge pull request #1402 from spxtr/ips120gpib

spxtr force-pushed the ips120gpib branch 2 times, most recently from 6363043 to 0b13aa9 Compare November 29, 2018 01:49

astafan8 reviewed Nov 29, 2018

View reviewed changes

jenshnielsen reviewed Dec 4, 2018

View reviewed changes

spxtr force-pushed the ips120gpib branch from 0b13aa9 to 61f3988 Compare December 11, 2018 03:46

jenshnielsen reviewed Dec 11, 2018

View reviewed changes

qcodes/instrument_drivers/oxford/IPS120.py Outdated Show resolved Hide resolved

jenshnielsen reviewed Dec 11, 2018

View reviewed changes

qcodes/instrument_drivers/oxford/IPS120.py Outdated Show resolved Hide resolved

IPS120: Support both GPIB and serial.

bf350bf

Also bring the driver up to date in terms of style.

spxtr force-pushed the ips120gpib branch from 8b664d7 to bf350bf Compare December 11, 2018 23:27

Merge branch 'master' into ips120gpib

192e2db

astafan8 requested changes Jan 17, 2019

View reviewed changes

Respond to review comments. Test on a real IPS120.

bfed143

astafan8 approved these changes Feb 13, 2019

View reviewed changes

qcodes/instrument_drivers/oxford/IPS120.py Show resolved Hide resolved

qcodes/instrument_drivers/oxford/IPS120.py Show resolved Hide resolved

Merge branch 'master' into ips120gpib

4948acd

astafan8 mentioned this pull request Feb 13, 2019

Oxford IPS120: Fix the return of a few methods for the magnet #985

Closed

astafan8 and others added 2 commits February 13, 2019 16:03

Merge branch 'master' into ips120gpib

c4660b4

Merge branch 'master' into ips120gpib

67fe64e

Merge branch 'master' into ips120gpib

36f4d99

astafan8 merged commit 46661dc into microsoft:master Feb 18, 2019

giulioungaretti pushed a commit that referenced this pull request Feb 18, 2019

Generated gh-pages for commit 46661dc

0de6a3c

Merge: 18054f5 36f4d99 Author: Mikhail Astafev <astafan8@gmail.com> Merge pull request #1402 from spxtr/ips120gpib

spxtr deleted the ips120gpib branch February 18, 2019 21:57

spxtr mentioned this pull request Jun 25, 2019

Two fixes for IPS120 driver. #1619

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various IPS120 driver improvements #1402

Various IPS120 driver improvements #1402

spxtr commented Nov 28, 2018

codecov bot commented Nov 29, 2018 •

edited

astafan8 left a comment

jenshnielsen left a comment

jenshnielsen Dec 4, 2018

spxtr Dec 11, 2018

spxtr commented Dec 11, 2018

jenshnielsen commented Dec 11, 2018

jenshnielsen commented Jan 16, 2019

spxtr commented Jan 16, 2019

astafan8 left a comment

astafan8 Jan 17, 2019

astafan8 left a comment

spxtr commented Feb 17, 2019

codecov bot commented Feb 18, 2019

Various IPS120 driver improvements #1402

Various IPS120 driver improvements #1402

Conversation

spxtr commented Nov 28, 2018

codecov bot commented Nov 29, 2018 • edited

Codecov Report

astafan8 left a comment

Choose a reason for hiding this comment

jenshnielsen left a comment

Choose a reason for hiding this comment

jenshnielsen Dec 4, 2018

Choose a reason for hiding this comment

spxtr Dec 11, 2018

Choose a reason for hiding this comment

spxtr commented Dec 11, 2018

jenshnielsen commented Dec 11, 2018

jenshnielsen commented Jan 16, 2019

spxtr commented Jan 16, 2019

astafan8 left a comment

Choose a reason for hiding this comment

astafan8 Jan 17, 2019

Choose a reason for hiding this comment

astafan8 left a comment

Choose a reason for hiding this comment

spxtr commented Feb 17, 2019

codecov bot commented Feb 18, 2019

Codecov Report

codecov bot commented Nov 29, 2018 •

edited