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

Add full type checking of Qdac drivers #2341

Merged
merged 8 commits into from
Nov 12, 2020

Conversation

jenshnielsen
Copy link
Collaborator

Should be merged after #2336 and not backported. This probably should be tested manually on both devices

@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #2341 (36aeac6) into master (6eae703) will increase coverage by 0.00%.
The diff coverage is 67.92%.

@@           Coverage Diff           @@
##           master    #2341   +/-   ##
=======================================
  Coverage   62.32%   62.32%           
=======================================
  Files         200      200           
  Lines       26267    26276    +9     
=======================================
+ Hits        16370    16377    +7     
- Misses       9897     9899    +2     

@jenshnielsen jenshnielsen force-pushed the qdac_type_checking branch 2 times, most recently from 9fb2dd1 to e2b860b Compare October 26, 2020 14:29
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 approve as long as the change has been tested on the insturment (including the verbose mode :) )

@FarBo
Copy link
Contributor

FarBo commented Nov 11, 2020

I will test it with the instrument and approve it

@FarBo FarBo self-requested a review November 11, 2020 11:07
@FarBo
Copy link
Contributor

FarBo commented Nov 12, 2020

@jenshnielsen

I tested this PR for Qdevil. It seems that it is working without any issue (tested instrument version is 1.07). For testing _assigned_triggers, the only requirement is to ramp_voltage for more than 1 channel, otherwise triggers is 1 and _assigned_triggers will not be populated. Then, setting slope to 'Inf' for each channel results in popping that channel out of the _assigned_triggers.

@astafan8
What should be done for verbose mode? The comments in the driver is not that informative to what to do. Especially in the QDac docstring, it says "The driver assumes that the instrument is ALWAYS in verbose mode OFF
and sets this as part of the initialization, so please do not change this"

@jenshnielsen
Copy link
Collaborator Author

@FarBo thanks for testing

@jenshnielsen
Copy link
Collaborator Author

Perhaps @astafan8 meant update_currents=True ?

@FarBo
Copy link
Contributor

FarBo commented Nov 12, 2020

update_currents=True queries the instrument for all channels and return responses, which can be seen as DEBUG messages in the load time.

Copy link
Contributor

@FarBo FarBo left a comment

Choose a reason for hiding this comment

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

Both Qdac and Qdevil tested with the instruments. I approve this PR, unless @astafan8 explicitly explains what he wants to be tested in verbose mode.

@astafan8
Copy link
Contributor

unless @astafan8 explicitly explains what he wants to be tested in verbose mode.

The verbose mode allows one to test this change (the square brackets got turned into curly brackets):

https://github.com/QCoDeS/Qcodes/blob/36aeac67a27dd9630577705b531988e080c0cad7/qcodes/instrument_drivers/QDevil/QDevil_QDAC.py#L573-L575

Especially in the QDac docstring, it says "The driver assumes that the instrument is ALWAYS in verbose mode OFF
and sets this as part of the initialization, so please do not change this"

well, what can i say.... then no need to test the functionality that is not even supposed to be used by anyone apparently... :)

@jenshnielsen jenshnielsen merged commit 5b81db1 into microsoft:master Nov 12, 2020
@jenshnielsen jenshnielsen deleted the qdac_type_checking branch November 12, 2020 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants