Conversation
Implement actuator module
* ENH: seed sensor measurement noise per instance Sensor noise (white noise and random-walk bias drift in InertialSensor and ScalarSensor, plus the GNSS position/velocity accuracy) was drawn from the global numpy RNG. That made it impossible to reproduce from a seed, and unsafe under multiprocess where workers share the global state. Add a seed argument to the sensor constructors and draw all noise from a per-instance numpy Generator. seed=None keeps the random-by-default behaviour but per instance, so it no longer touches the global RNG, matching how StochasticModel and MonteCarlo already seed. A freshly-constructed sensor replays the same sequence, so reproducibility lives at construction rather than in _reset. Adds tests/unit/sensors/test_sensor_seeding.py. * TST: seed the noisy_barometer fixture to fix a pre-existing flake test_noisy_barometer asserts a barometer reading within rel=0.03 of the clean pressure, but the reading carries an unseeded Gaussian noise draw plus a +1000 Pa constant_bias the expected value omits, leaving only ~+2.42 sigma of headroom, so it already flaked at ~0.76% on develop (a 20k-trial check: develop 0.77%, this branch 0.75%). Passing a fixed seed makes it deterministic and also exercises the new sensor seed argument.
Following @zuorenchen's review note on #13, seed the remaining noisy sensor fixtures (noisy_rotated_accelerometer, noisy_rotated_gyroscope) the same way noisy_barometer already is, so the whole sensor suite is deterministic instead of relying on statistical tolerance bounds. Uses the seed argument added in #13.
* test: characterization tests for Flight.step_simulation() Verify the stepped-simulation API (run_simulation=False plus repeated step_simulation() calls) reproduces a one-shot simulate(): - initial step state is unfinished at the first phase - stepping visits multiple phases and reaches the finished state - the stepped trajectory (final t and the full solution array) matches simulate() to a tight tolerance, robust to LSODA last-bit noise across platforms - post_process_simulation / initialize_prints_plots fire on finish (t_final, prints, plots present) - stepping after finished is a no-op Uses the parachute-free calisto flight (parachute triggers are not migrated into the stepping path); the twin's launch parameters are read back from the reference so it cannot drift. Scope: uncontrolled stepping only. * test: cover controlled stepping in step_simulation() Inject a roll command between step_simulation() calls (the Balloon Popping Challenge use case) and assert the trajectory responds: - a sustained command spins the body up (roll rate w3 grows) while a neutral command leaves w3 at zero - a mid-flight command reversal turns the roll rate around -- only possible if the command is re-read every step (a latched command could not) - a zero command leaves the angular state bit-identical to an uncontrolled run, isolating the divergence as the command, not the actuator's presence Uses time_overshoot=False (as BPC does) so each step advances one solver node and the command is injected per timestep. Tolerances/thresholds are used rather than bit-exact equality, to stay robust to LSODA last-bit noise across platforms.
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>
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>
@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>
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>
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>
Validate actuator arguments so the checks survive -O and None works
# Conflicts: # CHANGELOG.md
This was referenced Jul 29, 2026
thc1006
added a commit
that referenced
this pull request
Jul 29, 2026
`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>
This file contains hidden or 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
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.
Hotfix release for v1.13, as agreed in #18.
The only change over
masteris #19: the actuator argument checks survivepython -O, a non-finite command is refused at the setter rather than reaching the integrator,add_*_controlhands the controller the same sampling rate the actuator kept, and a range that is not a pair of numbers is refused.developis 42 commits ahead ofmaster, but the tree difference is these four files: everything else already reachedmasterwith v1.13 itself.CI was green on all eight checks at
bd9317bc, andtest_actuators.pyis 126 tests, up from 88.Downstream: BalloonPoppingChallenge pins this repository's
master, so this is what lets that bump happen. Its own action validation lands with the bump, so a competitor's non-finite action keeps being dropped rather than becoming an exception.