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

Driver dev/switch matrix #1373

Merged
merged 34 commits into from
Nov 24, 2018
Merged

Driver dev/switch matrix #1373

merged 34 commits into from
Nov 24, 2018

Conversation

StefanD986
Copy link
Contributor

Adds driver and example notebook for Keysight B2200 Femto leakage switch Matrix

Changes proposed in this pull request:

  • Example Notebook under docs/examples/driver_examples/Qcodes_example_with_Keysight_B2200_femto_leakage_switch_matrix.ipynb
  • Driver under qcodes/instrument_drivers/Keysight/keysight_b220x.py
  • Tests under qcodes/tests/drivers/test_keysight_b220x.py

@astafan8

…tead of disconnect). This adds a test for the issue and fixes the problem.
@sohailc
Copy link
Member

sohailc commented Nov 5, 2018

Great work, @StefanD986! I would suggest:

  • Add type hints
  • Check that some lines of code are not overly long. I think the standard is 80 characters.

@jenshnielsen
Copy link
Collaborator

@StefanD986 Thanks for the contribution. There is a few small things you need to do to get the docs to build correctly. For this to work there are some requirements for the structure of the notebook. Please ensure that the notebook starts with a single level 1 heading (A single #) with the title of the notebook. All other titles should be level 2 (##) or 3 (###) taking care that level 3 headings only appear within level2 headings. Could you also rename the notebook file so that it is consistent with the other ones in https://github.com/QCoDeS/Qcodes/tree/master/docs/examples/driver_examples

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.

That is a well-written driver, and an outstanding example notebook! Thank you for the quality! Users will be very happy!

I request changes in order to get the usage of add_function removed, get pep8 lines length, and get typos fixed.

qcodes/tests/drivers/test_keysight_b220x.py Show resolved Hide resolved
qcodes/tests/drivers/test_keysight_b220x.py Outdated Show resolved Hide resolved
qcodes/tests/drivers/test_keysight_b220x.py Outdated Show resolved Hide resolved
qcodes/tests/drivers/test_keysight_b220x.py Show resolved Hide resolved
qcodes/instrument_drivers/Keysight/keysight_b220x.py Outdated Show resolved Hide resolved
astafan8 and others added 4 commits November 7, 2018 11:51
changes in this commit:
 - Wrong termination character was used.
 - in several places unclarities in the documentation/notebook were (hopefully) cleared up
 - deprecated usage of `add_function` was replaced with normal python methods.
 - at some places type hints were added.
 - in all commands the previously hardcoded `card` parameter was replaced with the private self._card. This is currently not used but will make it easier to add future functionality.
- several other small changes as requested in discussion of PR
… Matrix.ipynb

Changes heading levels in example notebook
StefanD986 and others added 8 commits November 21, 2018 17:37
status polling is added by using a decorator around the function calls. This is now only used for member functions, not for properties!
mypy doesn't seem like decorated functions. For now I simply removed the typehints, and added some docstrings instead.
Stack level of warning is changed to 2, so that the line of code which
caused the warning is displayed in the warning, instead of reporting
the line of code in the function which is used to check the status byte
and issue the warning.
@codecov
Copy link

codecov bot commented Nov 22, 2018

Codecov Report

Merging #1373 into master will increase coverage by 0.25%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1373      +/-   ##
==========================================
+ Coverage   73.52%   73.78%   +0.25%     
==========================================
  Files          79       80       +1     
  Lines        9267     9345      +78     
==========================================
+ Hits         6814     6895      +81     
+ Misses       2453     2450       -3

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.

Modulo minor comments, i think it's ready!

To please humans and machines alike this
- removes 1 unused import
- replaces str.format syntax with python 3.6 f-strings (thanks @astafan8 for the hint)
- some other small changes to please the linter
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.

I think this is ready to go. Last chance to confirm: does the driver work OK for the real instrument? :) if yes, then i'll merge.

@StefanD986
Copy link
Contributor Author

Didn’t see any smoke. Go ahead.

@astafan8 astafan8 merged commit c195dc8 into microsoft:master Nov 24, 2018
giulioungaretti pushed a commit that referenced this pull request Nov 24, 2018
Merge: 6d7cca6 48ea426
Author: Mikhail Astafev <astafan8@gmail.com>

    Merge pull request #1373 from StefanD986/driver_dev/SwitchMatrix
@StefanD986 StefanD986 deleted the driver_dev/SwitchMatrix branch November 25, 2018 09:08
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

4 participants