AMI430: fix coil_constant-related bugs of current- and field- parameters #1527
Conversation
has_current_rating argument should only matter for field_rating, not for field_ramp_limit.
astafan8
changed the title
This parameter is linked to current_limit paramater via coil_constant as a scale, so when coil_constant changes, the scale also needs to be updated.
This is more useful for tests.
Ensure that inter-dependent parameters (like coil-field-current) are all updated correctly when one of them gets updated.
Codecov Report
@@ Coverage Diff @@
## master #1527 +/- ##
==========================================
+ Coverage 72.03% 72.12% +0.09%
==========================================
Files 105 105
Lines 12304 12305 +1
==========================================
+ Hits 8863 8875 +12
+ Misses 3441 3430 -11 |
|
Did we ever figure out why this difference exists. Afaik it had something todo with the driver/docs version on AMIs webpage being out of date and the new versions only being on bitbucket? It would be good to at least update the comments about this. |
|
@jenshnielsen with the "difference" you mean the |
|
That comment still reflects my knowledge, unfortunately. But I have not compared firmware versions of different models, so there is still the possibility that we'll be able to figure out roughly in what firmware version that feature was added/removed. |
|
This PR will hopefully be tested next week with @damazter2 in Delft. |
many times, different values. for robustness
.. in case ramp_rate is higher than the new limit
|
Thanks to @damazter @damazter2 rigorous testing of this change, we now confirm that the coil-constant related bugs are solved. By request of Damaz, two tests were added: one that runs the coil-constant/field/current-limit tests with arbitrary values and a number of times, and one new test for the fact that ramp_rate is reduced when ramp_limit is made lower than the current (now) ramp_rate. After CI shows green, I am going to merge this. @QCoDeS/core any objections? |
| warnings.simplefilter('ignore', category=AMI430Warning) | ||
|
|
||
| test_current_and_field_params_interlink_at_init(ami430) | ||
|
|
There was a problem hiding this comment.
Would it make sense to make this a bit more DRY by looping over 3 tuples or pytest parametrize ?
There was a problem hiding this comment.
sure. i can probably hard-code some values and use permutations from itertools or smth.
but only if you insist :)
There was a problem hiding this comment.
@jenshnielsen ? :)
There was a problem hiding this comment.
I would prefer but I don't insist. That's why its a question :)
Fixes:
has_current_ratingargument/attribute should only matter forfield_ratingparameter, not forfield_ramp_limitparameter.scaleoffield_limitparameter when acoil_constantis setramp_rateif newcurrent_ramp_limitorfield_ramp_limitis lower thanramp_rateThis PR might end up resolving #1525.