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

Feature/keithley 2000 scan #85

Merged
merged 12 commits into from
Jan 15, 2021

Conversation

marcelhohn
Copy link
Contributor

New features:

  • added class for Keithley 2000-SCAN card
  • added scanning card to Keithely_6500 class
  • Keithley 2000-SCAN can be included into other DMM classes if necessary
  • Keithley DMM6500: Check if correct terminal is activated
  • Keithley DMM6500: Allow for simpler access of parameters (e.g. dmm.resistance.get() instead of dmm.measure.res.get(). The latter still works to ensure "downwards" compatibilty.)

@FarBo
Copy link
Contributor

FarBo commented Dec 16, 2020

@MarceloSemO

Thank you for the contribution. I will start the review ASAP.

@FarBo
Copy link
Contributor

FarBo commented Dec 18, 2020

I have kind of general comments from skimming in the files:

  1. Please provide a little bit of docstring for classes and functions (ideally, all arguments in the classes and functions could have some descriptions)
  2. All defined parameters in both .py files should have validators specifying valid data for that parameter and min/max if required (you can find validators in qcodes.utils.validators)
  3. Please provide typing for all arguments

@marcelhohn marcelhohn force-pushed the feature/Keithley_2000-SCAN branch 3 times, most recently from c4ad53a to 41ae3a9 Compare December 22, 2020 11:07
@marcelhohn
Copy link
Contributor Author

marcelhohn commented Dec 22, 2020

Thanks for the feedback, I added docstrings and typings. Two problems/questions are remaining:

  1. Should parameter which are only gettable also have a validator? I am asking because all settable parameters in my drivers already have validators, but you still asked me to add validators to the parameters. As far as I understood, the purpose of a validator is to prevent to send values which could cause unpredictable behavior of the device. In case of non-settable parameters this cannot happen anyways. Please correct me if I am mistaken and if I should also add validators to non-settable parameters.

  2. The test now fails, because the autodoc fails to import the module Keithley_2000_Scan. I don't know how to solve this, since the only import of the Keithley_2000_Scan module happens in the Keithley_6500 module, and I did not change the import statement there. What I changed is that I imported the Keithley_6500 module in the Keithley_2000_Scan module, since I needed this to provide the desired type hint for the variable "dmm" in the __init__ function of the Keithley_2000_Scan_Channel class. My guess is that the circular import is what causes the test to failm but I don't know how to fix, so I would appreciate your help.

@jenshnielsen
Copy link
Collaborator

@MarceloSemO I pushed a small fix for the circular import. Basically you need to only import the Parent instrument into the scan module when you are typechecking

Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

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

I agree that validators are not needed for get only parameters. I left a few suggestions inline

@FarBo
Copy link
Contributor

FarBo commented Dec 22, 2020

@MarceloSemO

Regarding the first question, there is no need for validators for only-gettable parameters.

Thanks for the changes. I am reviewing it in a continuous basis.

@marcelhohn marcelhohn force-pushed the feature/Keithley_2000-SCAN branch from 40021eb to 4350928 Compare December 24, 2020 12:32
@marcelhohn
Copy link
Contributor Author

@jenshnielsen and @FarBo, thank you very much for your comments! I have implemented the changes you suggested and performed a rebase with respect to the latest commit on the master branch. I think the pull request should now be ready to be merged.

@marcelhohn marcelhohn force-pushed the feature/Keithley_2000-SCAN branch from 05eebd2 to 4350928 Compare January 15, 2021 12:19
Marcel Hohn and others added 12 commits January 15, 2021 14:28
- undo renaming of keyword argument 'channel'
- correct docstring for parameter 'NPLC'
- rename private functions ('measure' -> '_measure')
- use full path for InstrumentChannel import
- raise error if wrong terminal is active
The method Keithley.measure was renamed to Keithley._measure in commit 4350928.
In the class Keithley_Sense, however, the method call to this method has not been
renamed accordingly. This commit fixes this error.
@marcelhohn marcelhohn force-pushed the feature/Keithley_2000-SCAN branch from d387d54 to 683cc86 Compare January 15, 2021 13:28
@marcelhohn
Copy link
Contributor Author

I have now re-run the notebook and implemented the required changes. I also performed a rebase, now the PR should be ready to be merged.

@jenshnielsen jenshnielsen merged commit 7ca17a3 into QCoDeS:master Jan 15, 2021
@marcelhohn marcelhohn deleted the feature/Keithley_2000-SCAN branch February 3, 2021 17:51
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.

3 participants