ENH: Discrete and Continuous Controllers#946
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #946 +/- ##
===========================================
+ Coverage 80.27% 81.07% +0.80%
===========================================
Files 104 113 +9
Lines 12769 14555 +1786
===========================================
+ Hits 10250 11801 +1551
- Misses 2519 2754 +235 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Malmahrouqi3
force-pushed
the
enh/discrete_and_contrinuous_control
branch
from
538aca0 to
a4de476
Compare
There was a problem hiding this comment.
Pull request overview
Adds first-class support for discrete (fixed refresh rate) and continuous (called every solver step) controllers to RocketPy’s flight simulation, aligning with the controller architecture discussed in issue #274.
Changes:
- Invoke continuous controllers during the solver stepping loop (instead of via fixed time nodes).
- Add
Rocket.add_discrete_controllerandRocket.add_continuous_controllerhelpers that build_Controllerobjects with finite vsinfsampling rates. - Update
_Controllerto defaultsampling_ratetomath.infand document the “infinite sampling” behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
rocketpy/simulation/flight.py |
Calls continuous controllers each solver step and skips time-node creation for sampling_rate=inf. |
rocketpy/rocket/rocket.py |
Adds Rocket-level APIs to register discrete vs continuous controllers. |
rocketpy/control/controller.py |
Defaults controller sampling rate to infinity and updates documentation accordingly. |
Comments suppressed due to low confidence (1)
rocketpy/control/controller.py:81
- The added note in the docstring exceeds the typical line-length used elsewhere in the project (and likely will be reformatted by Ruff/Black). Please wrap this note to keep docstring formatting consistent/readable, and consider clarifying how
sampling_rate=infinteracts with the existing1/sampling_ratewording in thesampling_rateparameter description.
.. note:: The function will be called according to the sampling rate
specified. If unspecified, the default sampling rate is set to infinity, meaning that the
controller function will be called at every step of the simulation.
sampling_rate : float
The sampling rate of the controller function in Hertz (Hz). This
means that the controller function will be called every
`1/sampling_rate` seconds.
MateusStano
left a comment
MateusStano
left a comment
There was a problem hiding this comment.
Really clean! Just requested a couple of changes and it should be good to merge
| def add_discrete_controller( | ||
| self, | ||
| controller_function, | ||
| refresh_rate, | ||
| interactive_objects=None, | ||
| initial_observed_variables=None, | ||
| name="Controller", | ||
| ): | ||
| """Creates a new discrete controller, storing its parameters such as | ||
| controller function, refresh rate, and interactive objects. The controller | ||
| will be called at the specified refresh rate during the simulation.""" | ||
|
|
||
| controller = _Controller( | ||
| controller_function=controller_function, | ||
| sampling_rate=refresh_rate, | ||
| interactive_objects=interactive_objects, | ||
| initial_observed_variables=initial_observed_variables, | ||
| name=name, | ||
| ) | ||
|
|
||
| self._add_controllers(controller) | ||
|
|
||
| def add_continuous_controller( | ||
| self, | ||
| controller_function, | ||
| interactive_objects=None, | ||
| initial_observed_variables=None, | ||
| name="Controller", | ||
| ): | ||
| """Creates a new continuous controller, storing its parameters such as | ||
| controller function and interactive objects. The controller will | ||
| be called at every time step of the simulation.""" | ||
|
|
||
| controller = _Controller( | ||
| controller_function=controller_function, | ||
| sampling_rate=math.inf, | ||
| interactive_objects=interactive_objects, | ||
| initial_observed_variables=initial_observed_variables, | ||
| name=name, | ||
| ) | ||
|
|
||
| self._add_controllers(controller) | ||
| return controller |
There was a problem hiding this comment.
Because controllers are still an experimental feature, I´d prefer to only have self._add_controllers for now. This way we keep the interface between Controller and Rocket private and we can make changes as the architecture evolves. For this reason, please remove add_discrete_controller and add_continuos_controller
| interactive_objects, | ||
| controller_function, | ||
| sampling_rate, | ||
| sampling_rate=math.inf, |
There was a problem hiding this comment.
Is there a specific reason for using math.inf here? I think using None to indicate a continuous signal is cleaner and safer. It prevents sampling_rate from being mistakenly evaluated as a number when it doesn't have a meaningful value
Pull request type
Checklist
black rocketpy/ tests/) has passed locallyCurrent behavior
#274
New behavior
Tested on the Airbrakes Example (
docs/user/airbrakes.rst)Breaking change
Additional information
There is no node creation for continuous controllers hence this step is skipped in
flight.py. Instead, they are called in each of the ODE solver step, hence not relying on fixed sampling rate.