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.
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