AMI430 driver setting set point despite being outside safe region #1162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ments and then update _set_point. 2) get rid of "_set_x", "_set_y", etc. Instead have a single "_set_setpoints" method 3) Make the "test_ramp_down_first" more robust by not checking time stamps any more 4) In the test which tests field limits assert that the set point is not set when an error is raised.
sohailc
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1162 +/- ##
==========================================
+ Coverage 79.71% 79.78% +0.06%
==========================================
Files 48 48
Lines 6686 6662 -24
==========================================
- Hits 5330 5315 -15
+ Misses 1356 1347 -9 |
astafan8
approved these changes
Jun 27, 2018
| @@ -570,90 +570,88 @@ def __init__(self, name, instrument_x, instrument_y, | |||
| # Get and set parameters for the set points of the coordinates | |||
| self.add_parameter( | |||
| 'cartesian', | |||
| get_cmd=partial(self._get_setpoints, 'x', 'y', 'z'), | |||
| set_cmd=self._set_cartesian, | |||
| get_cmd=partial(self._get_setpoints, ['x', 'y', 'z']), | |||
There was a problem hiding this comment.
Mutable default arguments to functions always makes me slightly nervous. These are never modified so you could make them tuples right?
There was a problem hiding this comment.
mutable defaults? Where do you see that? I guess I can change that to a tuple without problems
There was a problem hiding this comment.
Or will partial create a function with a list as default arguments? I did not think of that... :-)
There was a problem hiding this comment.
@sohailc yes that's my thinking, It's a small issue for sure since this never modified the input arg
…i_invalid_field_limits_set
…/sohailc/Qcodes into fix/ami_invalid_field_limits_set
…i_invalid_field_limits_set
…ults 2) Cleaned up field vector
giulioungaretti
pushed a commit
that referenced
this pull request
Jul 17, 2018
Merge: 16568f4 73306cf Author: sohail chatoor <[email protected]> Merge pull request #1162 from sohailc/fix/ami_invalid_field_limits_set
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solves issue #1143
The problem was that the set point vector was adjusted before testing that the set point was safe.