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

Lakeshore Model 336: make runnable, add minimal test, and fixes along the way #1526

Merged

Conversation

astafan8
Copy link
Contributor

This PR is a result of a wish of using Lakeshore Model 336 driver in Delft, which turned into fixing some bugs in the drivers.

Most notable:

  • Add test for Model 336 that is very similar to test for Model 372 (quick copy-paste mostly)
  • Fix BaseOutput's input_channel parameter creation. It has to be a group parameter, but other kwargs like val_mapping and/or vals and others can be now specified in the subclasses to allow aproprivate variations between models. in Lakeshore Model 336, Model 372, and their base class.
  • Map input_channel values and sensor channel names. Add map between BaseOutput's input_channel values and sensor channel object names. Model 336 uses letters for sensor channel names, hence the values of the BaseOutput's input_channel parameter naturally become strings. Model 372 uses numbers (it has 16 channels, while 366 has 4) for sensor channels, hence the values of the BaseOutput's input_channel parameter naturally become integers. To bridge this difference, and make wait_until_set_point_reached work for any Lakeshore model driver, a new mapping with a long ugly name input_channel_parameter_values_to_channel_name_on_instrument is introduced.
  • Fix set_range_from_temperature bug for upper range - If the given temperature is from the highest range, the logic in the function would throw an exception because the index that bisect returns
    is +1 relative to what it should be. Now it's fixed, and a test is added. For Lakeshore drivers, all models

@QCoDeS/core This PR "as is" does not act on the fact that the driver and test infrastructure for it is quite a bit convoluted and difficult to wrap ones head around. Hence, this PR only makes fixes, so that Model 336 driver works. Would you insist on reworking the structure of the driver and the test infrastructure in this PR or shall we leave it for "later"?

... becasue it is parametrized correctly in the BaseOutput class

Lakeshore Model 336 driver
It has to be a group parameter, but other kwargs like
val_mapping and/or vals and others can be now specified
in the subclasses to allow
aproprivate variations between models.

in Lakeshore Model 336, Model 372, and their base class
Add map between BaseOutput's input_channel values and sensor channel
object names. Model 336 uses letters for sensor channel names,
hence the values of the BaseOutput's input_channel parameter
naturally become strings.
Model 372 uses numbers (it has 16 channels, while 366 has 4) for sensor channels,
hence the values of the BaseOutput's input_channel parameter
naturally become integers.
To bridge this difference, and make wait_until_set_point_reached work
for any Lakeshore model driver,
a new mapping with a long ugly name
input_channel_parameter_values_to_channel_name_on_instrument
is introduced.

This will be refactored later perhaps.
If the given temperature is from the highest range,
the logic in the function would throw an exception
because the index that bisect returns
is +1 relative to what it should be.
Now it's fixed, and a test is added.

For Lakeshore drivers, all models
It is mostly copy-paste of Lakeshore Model 372 test.

The test infrastructure will be improved in later commits.
@astafan8 astafan8 marked this pull request as ready for review March 22, 2019 16:40
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 think this looks quite sensible

qcodes/instrument_drivers/Lakeshore/lakeshore_base.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 1, 2019

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1526   +/-   ##
=======================================
  Coverage   70.66%   70.66%           
=======================================
  Files         102      102           
  Lines       11585    11585           
=======================================
  Hits         8186     8186           
  Misses       3399     3399

active_channel_name_on_instrument
@astafan8 astafan8 merged commit 2a4a80c into microsoft:master Apr 2, 2019
@astafan8 astafan8 deleted the lakeshore-336-minimal-test-and-fix branch April 2, 2019 10:01
giulioungaretti pushed a commit that referenced this pull request Apr 2, 2019
Merge: 7479f7a 3852460
Author: Mikhail Astafev <astafan8@gmail.com>

    Merge pull request #1526 from astafan8/lakeshore-336-minimal-test-and-fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants