Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added a driver for the Keithley 2450 #1533

Merged
merged 64 commits into from
May 1, 2019
Merged

Conversation

sohailc
Copy link
Member

@sohailc sohailc commented Apr 5, 2019

A driver for the Keithley 2450 has been added.

@codecov
Copy link

codecov bot commented Apr 5, 2019

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1533   +/-   ##
=======================================
  Coverage   71.33%   71.33%           
=======================================
  Files         104      104           
  Lines       12041    12041           
=======================================
  Hits         8589     8589           
  Misses       3452     3452

…g a warning and change the compatibility more to True. This will automatically raise a run time error as we are changing the language setting.
@sohailc
Copy link
Member Author

sohailc commented Apr 5, 2019

@WilliamHPNielsen @jenshnielsen @Dominik-Vogel @astafan8 Can someone look at my code, please?

@sohailc sohailc changed the title added a 'driver' for the 2450, which defaults to 2400. added a 'driver' for the 2450 Apr 15, 2019
@sohailc
Copy link
Member Author

sohailc commented Apr 25, 2019

@jenshnielsen @astafan8 I have improved my driver... can we have another look at it?

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks decent! Left some last comments.

And a couple of wishes for the future:

  • if the PR becomes too big, as in there are too many changes to be made, it is more convenient to close the existing PR, and open another one that does not have the previous comments. This is very much applicable in this case, because the driver was basically rewritten.
  • please try to respond to all the comments in one way or another. at least, use the "resolve conversation" button - this really helps to see which comments have already been addressed and which ones are still to be discussed or tackled.

qcodes/instrument_drivers/tektronix/Keithley_2450.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/tektronix/Keithley_2450.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/tektronix/Keithley_2450.py Outdated Show resolved Hide resolved
@jenshnielsen
Copy link
Collaborator

I am happy to have this merged once @astafan8 s comments have been addressed.

@sohailc sohailc merged commit 5a2ed14 into microsoft:master May 1, 2019
@sohailc sohailc deleted the keithley_2450 branch May 1, 2019 16:59
giulioungaretti pushed a commit that referenced this pull request May 1, 2019
Merge: d0466af 98e8787
Author: sohail chatoor <schatoor@gmail.com>

    Merge pull request #1533 from sohailc/keithley_2450
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants