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/dem 613/update s5i driver #1392

Merged
merged 7 commits into from
Nov 23, 2018
Merged

Feature/dem 613/update s5i driver #1392

merged 7 commits into from
Nov 23, 2018

Conversation

qSaevar
Copy link
Contributor

@qSaevar qSaevar commented Nov 22, 2018

Changes proposed in this pull request:

  • Update qutech S5i driver

@astafan8

@codecov
Copy link

codecov bot commented Nov 22, 2018

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1392   +/-   ##
=======================================
  Coverage   73.52%   73.52%           
=======================================
  Files          79       79           
  Lines        9267     9267           
=======================================
  Hits         6814     6814           
  Misses       2453     2453

@astafan8
Copy link
Contributor

@qSaevar Could you briefly describe in the description what you did and why? (so that I don't ask gazillion of questions inside the code) for example, "removed parameter X because Y", etc. (and next time, don't commit all changes at once - instead, make one atomic change like "removed parameter X because Y", commit it, and only then commit a different change i.e. "added parameter Z because G")

@qSaevar
Copy link
Contributor Author

qSaevar commented Nov 22, 2018

@astafan8 The underlying driver, S5i_module, has been updated and this PR is first and foremost addressing those changes. The use_external_reference method has been removed from the underlying driver, so I removed that parameter. Also the set_frequency_optimally was removed from S5i_module and replaced with get_optimal_stepsize so I updated _optimize accordingly.

But since I was changing the driver, I also added the enabled parameter that can be used to switch the module's output on and off. I added docstrings to the parameters and sharpened the vals to only allow what is possible in the underlying driver for power and frequency.

The updated driver has been tested on hardware.

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.

Thanks for the description! It's now very clear!

Please go through the code and fix the concerns (especially the major ones about flexibility).

qcodes/instrument_drivers/QuTech/S5i.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/QuTech/S5i.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/QuTech/S5i.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/QuTech/S5i.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/QuTech/S5i.py Show resolved Hide resolved
qcodes/instrument_drivers/QuTech/S5i.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/QuTech/S5i.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/QuTech/S5i.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/QuTech/S5i.py Outdated Show resolved Hide resolved
@qSaevar
Copy link
Contributor Author

qSaevar commented Nov 22, 2018

I addressed the comments, all but one, as setting limits on the stepsize is dependent on the set frequency.

@qSaevar
Copy link
Contributor Author

qSaevar commented Nov 22, 2018

And for good measure we would like to test this driver again on hardware before it is merged.

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.

This looks much better! Nevertheless, please, see the comment about enable_output kwarg and parameter.

qcodes/instrument_drivers/QuTech/S5i.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/QuTech/S5i.py Outdated Show resolved Hide resolved
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.

Great! Once the testing on the actual instrument is passing, let know, i'll merge.

@quantumkoen
Copy link

@astafan8 i have tested the latest changes on this branch against our setup, and can confirm that all is working as expected.

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

    Merge pull request #1392 from qutech-sd/feature/DEM-613/Update-S5i-driver
@qSaevar qSaevar deleted the feature/DEM-613/Update-S5i-driver branch November 27, 2018 07:41
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