Add function to instrument to validate all gettable parameters#347
Add function to instrument to validate all gettable parameters#347
Conversation
AdriaanRol
left a comment
AdriaanRol
left a comment
There was a problem hiding this comment.
@peendebak I like this addition 👍 , I have some small questions regarding the tests but other than that I think it should be ok to merge (leaving the final decision to @giulioungaretti of course)
qcodes/tests/test_instrument.py
Outdated
| instrument.dac1._save_val(1000) # overrule the validator | ||
| try: | ||
| instrument.validate_status() | ||
| except: |
There was a problem hiding this comment.
This is a blind except, I would always use except Exception: to prevent keyboard interrupts and the like to be caught here.
qcodes/tests/test_instrument.py
Outdated
| # TODO (giulioungaretti) remove ( does nothing ?) | ||
| pass | ||
|
|
||
| def validate_function(self): |
There was a problem hiding this comment.
If you start the function name with test_validate_function unittest will run this function. I'm not sure, did you test that this runs (I did not so I may be wrong)
qcodes/tests/test_instrument.py
Outdated
| instrument.validate_status() | ||
| except: | ||
| pass | ||
| else: |
There was a problem hiding this comment.
unittest contains a with self.assertRaises(Exception): construction where you can test for a specific exception without the need for the try except else construction.
peendebak
force-pushed
the
feat/validate_status
branch
from
f5dc71b to
3b0a34f
Compare
|
@AdriaanRol Thanks for the suggestions. |
|
@AdriaanRol @giulioungaretti For the review of Adriaan I have two options: see review or dismiss review. I would have expected an option like `handled review'. Should I dismiss the review if I feel the issues have been addressed? |
| cls.source = MockSource(model=cls.model, server_name='') | ||
| cls.meter = MockMeter(model=cls.model, keep_history=False, server_name='') | ||
| cls.meter = MockMeter( | ||
| model=cls.model, keep_history=False, server_name='') |
There was a problem hiding this comment.
Is this acutally good PEP8 ? model should be aligned with the ( , no ?
There was a problem hiding this comment.
My autopep8 formatter says its fine
| 'only be used in Loops with background=False.') | ||
|
|
||
| def validate_status(self, verbose=0): | ||
| """ Validate the values of all gettable parameters """ |
There was a problem hiding this comment.
I'd prefer verbose to be a real bool, than the 0 (which suggests a level of verbosity rather than a switch. ).
And what happens if the parameter is gettable, but not settable ?
There was a problem hiding this comment.
The verbose parameter is an int on purpose. In general I use the convetion: 0: no output, 1: normal output, >=2: used for debugging, output not stable
There was a problem hiding this comment.
If the parameter is gettable but not settable, then nothing happens. The assumption here is that if there is no .set, then there is probably no meaningfull .validate.
If we want we can remove the check on has_set, since every parameter has a .validate function.
There was a problem hiding this comment.
a) Cool, just write the assumption in the docstring !
b) In this function 2 and 1 are the same then, which I don't like, and I don't see it documented or understandable from the code.
Change to true and false in this case.
There was a problem hiding this comment.
@giulioungaretti Ok, changes have been done in #369 which points to
4 participants
This is useful when connecting to an instrument for the first time to check that all values are in the required range.
@giulioungaretti @alexcjohnson