Validate actuator arguments so the checks survive -O and None works - #19
Conversation
Three problems in Actuator.__init__, all reported in #18 and all hit while building actuator integration tests for BalloonPoppingChallenge. demand_rate=None is the documented continuous-time mode, but the check read "demand_rate > 0 or demand_rate is None". Python evaluates the left operand first, so None > 0 raised TypeError and the mode could not be constructed at all. The four argument checks were bare asserts, which the interpreter drops under python -O. I confirmed that a negative time constant, a negative rate limit and an inverted range were all accepted in silence that way. These read as argument validation rather than internal invariants, so they now raise ValueError. actuator_initial_output was assigned without any range check, so an actuator could start somewhere its own output setter would never have put it, and _reset() restored that value on every simulation. It now follows the same policy the setter uses: clamped when clamp is True, warned about when it is False. The one behaviour change to watch is AssertionError becoming ValueError. The actuators are fork-only, so the blast radius is this repo's own tests, which are updated here, and BalloonPoppingChallenge, which does not catch either. I checked that the clamp cannot move BalloonPoppingChallenge: all three of its actuators take the default initial output, and 0.0 for gimbal and roll and 1.0 for throttle are inside their ranges, so nothing clamps and no warning fires. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
zuorenchen
left a comment
zuorenchen
left a comment
There was a problem hiding this comment.
Great, thanks. I haven't added the v1.13 tag. Let's merge this as hotfix for v1.13
The checks in the previous commit read as the inverted comparison, and inverting is not the same as negating once NaN is in play. Every ordered comparison against NaN is false, so `nan <= 0` is false and the value went through, where the assert being replaced asked `nan > 0` and rejected it. Four arguments regressed that way: demand_rate, actuator_range, actuator_rate_limit and actuator_time_constant. Negate the positive predicate instead, which keeps the assert's meaning exactly and leaves normal values alone. Refuse a NaN initial output for the same reason. np.clip returns NaN for NaN, so clamping would have stored it and _reset() would have restored it, and there is no direction to clamp it towards in any case. Drive both the in-process and the -O test from one table of invalid arguments, so an argument cannot be rejected in the default interpreter and accepted under -O. Each entry names the message it expects, so a case cannot pass on some other argument's check. Add the zero cases on both sides: demand_rate must refuse it, and the non-negative bounds must accept it, since that is the documented no-dynamics and no-rate-limit setting. Give the out-of-range warning a stacklevel so it is not reported against the same line in actuator.py for every caller, and say in the three public Rocket docstrings what the constructors now enforce. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Pylint was red on this branch. The finding is one line, not the five
negated comparisons it looks like from the outside:
C0117 unnecessary-negation
R0124 comparison-with-itself
not actuator_initial_output == actuator_initial_output
That is the NaN idiom, and it reads as a redundant comparison to a
reader and to pylint alike. Reverting it to `<=` would let NaN back in,
which is what the whole change is about, so it becomes a positive test
on the value instead.
Doing it that way settles two things the comparisons never named.
Infinity is refused where a finite number is required: a demand rate of
infinity makes the sampling period zero, which drives the IIR
coefficient to zero and freezes the output. And a value that is not a
number at all is refused there rather than a few lines later inside
np.clip. The range endpoints are deliberately left alone, since the base
default really is (-inf, inf), meaning an actuator with no range.
The same rule now guards the output setter, which is the more important
half. The constructor rejected a NaN and the setter accepted one on the
next timestep: np.clip returns NaN for NaN and both range comparisons
are false for NaN, so neither the clamped nor the unclamped branch
noticed and it was stored. That is the path an agent writes to.
BalloonPoppingChallenge assigns its actions straight into these setters,
and a policy that goes unstable emits NaN, which from there reaches the
forces, the moments and the integrator state.
Two smaller things found on the way. The -O subprocess helper rendered
NaN as source but not infinity, so the new cases would have died on
NameError while the test read the return code and called it a pass; it
covers both now. And the three checks are extracted, because inlining
them pushed __init__ over pylint's statement limit and suppressing that
would have been the wrong way round.
Also corrects a docstring that promised a feature nobody wrote.
Rocket.add_thrust_vector_control said initial_gimbal_angle could be a
per-axis tuple. ThrustVectorActuator2D hands its value to both
single-axis actuators unchanged and to_dict records only the x-axis
value, so an asymmetric pair could not survive a round trip either. It
used to be accepted and silently applied to both axes and now raises,
since the initial output is converted to a float. Corrected rather than
implemented: the feature belongs upstream rather than in this fork.
Verified by mutation: the setter accepting non-finite values, the check
narrowed back to NaN alone, and the demand rate bound dropped all fail.
pylint rocketpy/ tests/ docs/ rates 10.00/10, ruff check and format are
clean, and tests/unit/rocket passes 260.
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The checks added here guard what is passed in. Three things the actuator then derives from those arguments were left unguarded, and each one puts a non-finite number into the output the integrator reads. The range was checked for ordering only, so `(inf, inf)` passed. Clamping a finite initial output against it stores `inf`, and the next ordinary command lands on `(1 - alpha) * inf`, which is `0.0 * inf`, and stores NaN. `_reset()` then restores it at the start of every later flight. `(-inf, -inf)` is the same in the other direction, and both are reachable through `Rocket.add_throttle_control`. Endpoints still are not required to be finite, because an infinite bound is how this class says "unbounded on that side" and the base default really is `(-inf, inf)`. The weaker property is what matters and is what is now checked: clamping a finite value has to give back a finite one. Given `lower <= upper` that fails exactly when the lower bound is `+inf` or the upper is `-inf`, so one-sided ranges such as `(0, inf)` are unaffected. The endpoints are also copied into a new tuple. They were the caller's own object, so a list mutated after construction moved the range out from under a validation that had already run. The IIR coefficient forms `Ts = 1 / demand_rate` before combining it with the time constant, and that intermediate is not something either argument contains. A subnormal demand rate of 5e-324 is finite and positive and passes every check at the boundary, but `1.0 / 5e-324` is `inf` and `inf / (1.0 + inf)` is NaN, so the first finite command stores NaN and stays there. A time constant of 1e308 overflows the denominator the other way and pins alpha to zero, freezing the actuator where the answer is about 0.5. Writing the same expression as `1 / (1 + tau * rate)` avoids forming the intermediate at all. Over 2000 random pairs across the ranges a real configuration uses, the two agree to 2.2e-16. The third is in `rocket.py`. Each `add_*_control` built the actuator and the controller from the same argument, and only the actuator normalized it. With `sampling_rate="100"` the actuator held `100.0` and the controller held `"100"`, the call returned a rocket that looked complete, and the failure surfaced later inside Flight at `1 / controller.sampling_rate`. The controller now takes the actuator's normalized value. Refusing strings and bools outright is the other available fix and is a wider behaviour change than this needs. Thrust vector control reads that value off its x axis: `ThrustVectorActuator2D` holds no `demand_rate` of its own, only the two axes it builds from the argument. Its class docstring lists one, along with six other attributes it also never assigns. That gap is left alone here. Seven mutations, one per fix and one per call site, each fail a named test; a control mutation survives. Existing suites unchanged: unit 1945 passed with the four pre-existing test_sensitivity import failures, integration and acceptance 168 passed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Attacking the previous commit rather than trusting it. Writing the filter coefficient as `1 / (1 + tau * rate)` removed the overflow in `1 / rate`, and left one in `tau * rate`: the product goes infinite where neither factor does, `1 / inf` is 0, and an actuator whose coefficient is 0 never moves. Measured, tau=1e200 at 1e200 Hz holds its initial 0.5 against a command of 0.25 for the whole flight. That is the same silent freeze the demand-rate check further up exists to prevent, so it raises now. The bound is nowhere near anything real. A 0.01 s time constant at 100 Hz gives 0.5, and 10 s at 1000 Hz gives 1e-4. A small coefficient is what a slow actuator is; only zero is broken, and only overflow reaches it. Also restructure the ordering check in the range validator. It was written as `not lower <= upper` to reject a pair of NaNs, which every ordered comparison lets through. That reads as a double negative and pylint refuses it as C0117, which is what turned the Linters job red. Naming NaN first and then comparing says the same thing in the order a reader expects. Taking pylint's own suggestion verbatim would have been a bug: `lower > upper` is false for two NaNs. The linter gate is the exit code, which is a bitmask, and not the score. The score stays at 10.00/10 with a convention message present, which is how this got pushed red. The three I0021 useless-suppression notes in motor.py, sensor.py and stochastic_rocket.py are information messages, set no bit, and are left alone. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
Heads up that two commits landed after your approval, so this is not the diff you signed off on. Both came out of a re-read of the boundary rather than new review feedback. The checks here guard what is passed in. Three things the actuator derives from those arguments were still unguarded, and each one puts a non-finite number into the output the integrator reads:
The second commit attacks the first: writing the coefficient as Nine mutations, one per fix and one per call site, each fail a named test, and a control mutation survives. One thing I did not touch: If you would rather re-read before this goes in, say so and I will hold it. Superseded in part. The first two items above were taken back out at 36bda41 after your comment below, since neither needs a number anyone would set. The third stayed, and three further items in the same spirit were added at 2fa75e6. The PR body describes what the branch does now. |
@zuorenchen's point on reading this file is right, and it is worth writing down as a rule rather than a mood: a check earns its place if it stops an honest run being rejected, a dishonest one being accepted, or the program falling over for someone. Three of these did none of those. Gone: the range validator, which existed so `throttle_range=(inf, inf)` could not clamp a finite value to infinity; the guard on the filter coefficient overflowing to zero, which needs a time constant near 1e200; and the tests built around a subnormal demand rate of 5e-324. None of those is a number anyone sets. Their combined weight was 67 lines of implementation and 124 lines of tests against a reader who has to get past them to change anything real. What stays is what a mistake actually looks like. A controller that diverges and writes NaN or infinity into the actuator is refused at the setter, which was the original point of #18. A negative time constant, a negative rate limit and a non-positive demand rate are refused. So is a range whose lower bound is above its upper. And `add_*_control` hands the controller the sampling rate the actuator kept, so passing `"100"` from a config file cannot leave the two holding different types and fail later inside Flight. The coefficient keeps its one-expression form, `1 / (1 + tau * rate)`. That was not really about overflow: it is one expression instead of two, agrees with the old one to 2.2e-16 across the range a real actuator uses, and happens to give the right answer for a subnormal rate without anyone guarding it. Suite goes 122 -> 104. ruff and pylint clean, pylint exits 0. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
You are right, and I have taken those parts back out (36bda41). TLDR: dropped the range validator, the coefficient-overflow guard, and the tests built on a 5e-324 demand rate. 191 lines gone. What stays is a controller writing NaN into an actuator, a negative time constant or rate limit, and the The part I found genuinely useful was your framing of it. I had been treating "can this input break it" as the question, and the better one is "would anyone ever set this number". So I wrote the rule into the PR body rather than leaving it as taste:
That line would have excluded everything you objected to and kept everything I would still argue for, which is a good sign it is the right cut. Having it written down also means the next person does not need either of our backgrounds to apply it, which I think is your point about the barrier. Since then (2fa75e6): applying that same rule found three things on the other side of it, none of which needs an unusual number, only a typo in a config file.
Suite is 122 tests now, up from 104 after the trim, and each of the three has a mutation that fails a named test. One distinction I would still draw, and I would like your read on it. A competitor's AI finding a strange but legal way to fly is the competition working, and I agree it would be fun to watch. Someone editing the recorded result to claim balloons they never reached is a different thing, and the submission checks in BPC are aimed at that rather than at surviving odd inputs. If you would rather I hold those to a lighter standard too, say so and I will. |
These are on the other side of the line from the ones taken out last commit. Nothing here needs an unusual number, only a config file with a typo in it, and two of the three say nothing at all when it happens. `actuator_range` was read by index, so its shape was never checked. Three endpoints dropped the third in silence, one raised IndexError, a bare float raised TypeError, and string endpoints, which is what a YAML value without quotes handling looks like, failed inside np.clip with a ufunc loop error. It is now a pair of numbers or a ValueError naming the endpoint. Lists and numpy scalars still work, and an infinite bound still means unbounded. Each `add_*_control` removed the existing controller before building its replacement. A second call with a rejected argument then left the rocket carrying an actuator that simulation would never call, with no error after the one that caused it: measured, `sampling_rate=-1` on a second call took the controller list from one entry to zero while `throttle_control` stayed set. Both halves are built first now. `demand_rate=None` with a rate limit or a time constant was accepted with a warning and then ignored. Asking for a 0.1 s lag and getting an instant response is worth stopping for, and the subclass docstrings already say a demand rate is required for those options. It raises at construction now. Four mutations, one per fix, each fail a named test. 122 tests, up from 104. pylint exits 0: rocket.py was one line over its 3050 limit after this, so the comments came down to two lines each. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
Ready from my side. CI green on Since your approval on Taken out after your comment about this being too fine-grained: the range validator that refused Kept and added, all reachable from a config file with a typo in it: a controller writing NaN into an actuator, a negative time constant or rate limit, an One of those is worth calling out because my own earlier commits made it easier to hit: each No effect on BPC scores. Both shipped scenarios leave the three time constants 122 tests here, up from 88 on develop. Each fix has a mutation that fails a named test. Happy for you to merge whenever suits. After this lands, BPC needs |
YAML reads `yes` and `on` as True and `float(True)` is 1.0, so `sampling_rate: yes` became 1 Hz instead of 100 with nothing said. The previous revision of the test file pinned that as the contract, which is worse than leaving it unspecified. Refused now, the same way a boolean is already refused as a range endpoint. `float(10**400)` raises OverflowError, so one argument in the same validator came back as a different exception type from everything beside it. 126 tests, up from 122. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
REL: v1.13 hotfix, actuator argument validation (#19)
`docs.yml` triggered on a base of master only, so the only pull request that ever started it was develop into master. A docstring that does not build therefore lands on develop unremarked and first fails the merge that was meant to ship it, which is what #19 and #23 hit and why the four warnings in the commit before this one went unnoticed for three days. develop as well. The same paths filter, so it still only runs when something it reads has changed. This also makes the commit before it self checking: without this, a pull request into develop cannot start the job that would prove the warnings are gone, and the only evidence would be a measurement in the description. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Closes #18.
Actuatorchecked its arguments with bareassert.python -Ostrips those, so a negative time constant or rate limit was accepted in silence. The checks raise now.What is refused, and why each one is worth refusing
Every item here is reachable from a config file with a typo in it, and three of them said nothing at all when it happened.
value <= 0lets it through.actuator_rangethat is not a pair of numbers. It was read by index, so its shape was never checked: three endpoints dropped the third in silence, one raisedIndexError, a bare float raisedTypeError, and string endpoints, which is what an unquoted YAML value can become, failed insidenp.clipwith a ufunc loop error. Lists and numpy scalars still work, and an infinite bound still means unbounded.add_roll_control,add_thrust_vector_controlandadd_throttle_controlbuilt both halves from the same argument and only the actuator normalized it, sosampling_rate="100"left the actuator holding100.0and the controller holding"100". The call returned a rocket that looked complete and the failure surfaced later inside Flight at1 / controller.sampling_rate.demand_rate=Nonetogether with a rate limit or a time constant. Neither is implemented for a continuous actuator, and both were accepted with a warning and then ignored. Asking for a 0.1 s lag and getting an instant response is worth stopping for, and the subclass docstrings already say a demand rate is required for those options.Also in
rocket.py: eachadd_*_controlremoved the existing controller before building its replacement, so a second call with a rejected argument left the rocket carrying an actuator that simulation would never call. Measured:sampling_rate=-1on a second call took the controller list from one entry to zero whilethrottle_controlstayed set. Both halves are built first now, which matters more with the checks above in place, since there are now more ways for that second call to be rejected.The filter coefficient is written as
1 / (1 + tau * rate)rather than formingTs = 1 / ratefirst. One expression instead of two, and the two agree to 2.2e-16 across the time constants and rates a real actuator uses.What this deliberately does not do
An earlier revision also refused a range that cannot clamp anything finite, and a filter coefficient that overflows to zero. Both need numbers no configuration sets:
throttle_range=(inf, inf), or a time constant near 1e200. On @zuorenchen's reading they were the wrong kind of thorough, and I agree, so they came out along with the tests built around them, 191 lines in total.The rule I am using from here, so this is repeatable rather than a judgement call each time: a check earns its place if it stops an honest run being rejected, a dishonest one being accepted, or the program falling over for someone. Every item in the list above meets it.
(inf, inf)did not.Effect on BalloonPoppingChallenge
None on any score. Both shipped scenarios leave
gimbal_time_constant,roll_torque_time_constantandthrottle_time_constantatnull, so the coefficient branch never runs, and their ranges are ordinary finite numbers. Measured by pointing BPC's submodule at this branch: both golden masters pass unchanged, the full suite is 315 passed, anduv lock --checkstill resolves, so the bump needs no lockfile change.Checks
ruff check,ruff format --checkandpylintclean, pylint exiting 0. Unit 1950 passed, with the four pre-existingtest_sensitivityimport failures that reproduce with this branch stashed. Integration and acceptance 168 passed.test_actuators.pyis 122 tests. Every fix above has a mutation that fails a named test.