ENH: Refactor Flight class to improve time node handling and sensor/controllers #843
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Flight class's __simulate() method to improve readability and enable time overshoot functionality for controllers and sensors. The main changes include:
- Breaking down the monolithic
__simulate()method into smaller, focused methods for better maintainability - Enabling controllers and sensors to work with
time_overshoot=Trueoption, which was previously disabled - Optimizing simulation performance by allowing overshootable time nodes for sensors and controllers
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
rocketpy/simulation/flight.py |
Major refactoring of Flight simulation methods, breaking __simulate() into smaller helper methods and adding support for time overshoot with controllers/sensors |
tests/integration/test_plots.py |
Reduced number of test flight configurations from 16 to 4 for faster test execution |
tests/integration/test_flight.py |
Added new test for air brakes with time overshoot and renamed existing test for clarity |
tests/integration/test_environment.py |
Minor cleanup removing unused today variable |
tests/fixtures/flight/flight_fixtures.py |
Added new fixture for air brakes flight with time overshoot enabled |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| + (self.y_sol[2] - self.env.elevation) ** 2 | ||
| >= self.effective_1rl**2 | ||
| if self.__check_simulation_events(phase, phase_index, node_index): | ||
| break # Stop if |
There was a problem hiding this comment.
The comment on line 707 'Stop if' is incomplete and unclear. It should explain what condition causes the simulation to stop.
| break # Stop if | |
| break # Stop simulation if a termination event (e.g., end of flight, crash, or other defined event) is detected |
Copilot uses AI. Check for mistakes.
| # Add last time node (always skipped) | ||
| overshootable_nodes.add_node(self.t, [], [], []) | ||
|
|
||
| if len(overshootable_nodes) < 1: |
There was a problem hiding this comment.
[nitpick] The condition len(overshootable_nodes) < 1 should be simplified to not overshootable_nodes for better readability and consistency with Python idioms.
| if len(overshootable_nodes) < 1: | |
| if not overshootable_nodes: |
Copilot uses AI. Check for mistakes.
| overshootable_node.y_sol, | ||
| self.sensors, | ||
| ): | ||
| return False # Early exit, parachute not triggred |
There was a problem hiding this comment.
Misspelling in comment: 'triggred' should be 'triggered'.
| return False # Early exit, parachute not triggred | |
| return False # Early exit, parachute not triggered |
Copilot uses AI. Check for mistakes.
| # reset controllable object to initial state (only airbrakes for now) | ||
| for air_brakes in self.rocket.air_brakes: | ||
| air_brakes._reset() | ||
| # Note: time_overshoot now supports both controllers and sensors |
There was a problem hiding this comment.
[nitpick] This comment doesn't provide sufficient context about the change. It should explain what the previous limitation was and why this change is significant.
| # Note: time_overshoot now supports both controllers and sensors | |
| # Previously, time_overshoot only supported controllers. | |
| # Now, it supports both controllers and sensors, allowing for more flexible | |
| # simulation and monitoring of sensor-triggered events. This change enables | |
| # users to track overshoot conditions based on sensor data as well as controller actions. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Good work! I hope this is the beginning of having a much more readable and developer friendly Flight class.
My only concern is making sure the behavior is exactly the same as before, therefore I made a single important comment on the early return of parachute triggers just to make sure we are on the same page. Of course, all the tests being green is already a big step.
Aside from that, my other suggestions are more style/naming choices. I prefer having some more standardized names for some methods, but I would approve the PR in the current state even if these suggestions are not taken, since it already does loads for readability.
| self.solution, | ||
| self.sensors, | ||
| ) | ||
| self.__process_sensors_and_controllers_at_current_node(node, phase) |
There was a problem hiding this comment.
I don't mind much, since this is a private method naming. However, do you think there might be a more scalable / descriptive name here, so as to avoid adding words to it in the future: e.g. __process_sensors_and_controllers_and...? Having trouble naming a method may indicate it may do too much.
Actually, my suggestion would be going for a more standardized approach. I have never seen a fixed convention for this, but it is nice to have some internal consistency when naming checkers/process/handlers, my two cents would be:
handler: returns a boolean and operate on solution variables;process: can operate on solution variables, but does not return;checker: returns a boolean, but only reads solution variables (e.g. does not setup phases/nodes, I did not see any pure checkers on this PR).
self.__process_sensors(...)
self.__process_controllers(...)
if self.__handle_parachute_triggers(...):
break
while running:
if self.__handle_simulation_events(...):
break
if self.time_overshoot:
# I nested the if clauses for readability
# Both ways are equivalent and should perform the same
if self.__handle_overshoot(...):
break| self.y_sol, | ||
| self.sensors, | ||
| ): | ||
| return False # Early exit: parachute not deployed |
There was a problem hiding this comment.
Really not sure, since the code is complex, but shouldn't this be a continue (i.e. even if one trigger does not go off, there are other parachutes to check)?
|
|
||
| return False | ||
|
|
||
| def __check_simulation_events(self, phase, phase_index, node_index): |
There was a problem hiding this comment.
Since the three methods inside it are handlers, I would also call this one a handler.
| ) | ||
| return False | ||
|
|
||
| def __check_overshootable_parachute_triggers( |
There was a problem hiding this comment.
This method and the __handle_parachute_triggers are essentially identical (https://www.diffchecker.com/FYbfsMQf/) aside from the history rollback. They could be converted intro a single method with a boolean parameter to toggle the rollback.
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest tests -m slow --runslow) have passed locallyCHANGELOG.mdhas been updated (if relevant)Current behavior vs New behavior
Flight.__simulate()method is more readableControllers and Sensors Simulations should run faster!!
Breaking change
Additional information