-
Notifications
You must be signed in to change notification settings - Fork 301
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
Fix QDac for qcodes v0.8.0: inconsistent values for vrange #1844
Fix QDac for qcodes v0.8.0: inconsistent values for vrange #1844
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work cleaning up this :)
channel.vrange.cache(v_range) | ||
self._update_v_validator(channel, v_range) | ||
channel.irange.cache(i_range) | ||
channel.v.cache(v * 0.1 if v_range else v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't is possible to call self._set_vrange(chan, v_range)
here instead of self._update_v_validator(channel, v_range)
and channel.v.cache(v * 0.1 if v_range else v)
? i'm especially worried about duplicating the v * 0.1
part of the logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are speaking from my heart. I really wanted to refactor this and already started doing it. But I had to undo the refactoring. Now I have been thinking about it for a while and defined the right vocabulary, writing a github comment, almost in love-letter format, to you. After that I realized a good way of refactoring and implemented it ;-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great :)
Codecov Report
@@ Coverage Diff @@
## master #1844 +/- ##
==========================================
- Coverage 70.37% 70.37% -0.01%
==========================================
Files 152 152
Lines 18847 18847
==========================================
- Hits 13264 13263 -1
- Misses 5583 5584 +1 |
Co-Authored-By: Mikhail Astafev <astafan8@gmail.com>
Co-Authored-By: Mikhail Astafev <astafan8@gmail.com>
This code has now been tested including the actual voltage output: ch = qdac.ch41
def assert_equal(v):
assert np.isclose(ch.v.cache(), v, rtol=1e-2)
assert np.isclose(ch.v(), v, rtol=1e-2)
assert np.isclose(ch.v(), dmm.volt(), rtol=1e-2)
ch.vrange(0)
ch.v(1)
assert_equal(1)
ch.vrange(1)
assert_equal(0.1)
ch.vrange(0)
assert_equal(1)
ch.vrange(0)
ch.v(0.5)
assert_equal(0.5)
ch.vrange(1)
assert_equal(0.05) |
Fix QDac for qcodes v0.8.0: inconsistent values for vrange
This addresses an issues that came to light with the improvement of the cache in qcodes v0.8.0. The 'vrange' parameter was used inconsistently. (So was 'irange')
A lot of refactoring was necessary in order to find the bug, but it is a good time to do the refactoring since the new driver version needs to be tested on a real device anyway.
Ready for review, but not merging, since the hardware tests are missing.