Env/flight axial aceleration #876
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to add axial acceleration calculation to the Flight class, which represents the acceleration along the rocket's longitudinal axis. However, the PR includes several unrelated changes that significantly expand its scope beyond the stated purpose.
Key Changes:
- Added
axial_accelerationproperty to calculate acceleration along rocket's axis - Added corresponding unit test using dot product verification
- Unexpectedly restored previously deprecated export methods (
export_pressures,export_data,export_sensor_data,export_kml) with full implementations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| rocketpy/simulation/flight.py | Added axial_acceleration cached property; restored export methods that were previously moved to FlightDataExporter; removed self.apogee initialization; added json and simplekml imports; removed deprecated import |
| tests/unit/simulation/test_flight.py | Added parameterized test to verify axial_acceleration calculation against manual dot product computation at key flight times |
| self.out_of_rail_state = np.array([0]) | ||
| self.apogee_state = np.array([0]) | ||
| self.apogee = 0 | ||
| self.apogee_time = 0 |
There was a problem hiding this comment.
The removal of self.apogee = 0 initialization appears to be unintentional. This attribute is still used in the codebase (e.g., line 899 where self.apogee = self.apogee_state[2] is assigned). Without this initialization, accessing self.apogee before apogee is reached during flight simulation could raise an AttributeError.
This line should be restored to maintain proper initialization of instance attributes.
| self.apogee_time = 0 | |
| self.apogee_time = 0 | |
| self.apogee = 0 |
| import json | ||
| import math | ||
| import warnings | ||
| from copy import deepcopy | ||
| from functools import cached_property | ||
|
|
||
| import numpy as np | ||
| import simplekml | ||
| from scipy.integrate import BDF, DOP853, LSODA, RK23, RK45, OdeSolver, Radau | ||
|
|
There was a problem hiding this comment.
The addition of import json and import simplekml at the module level, along with the removal of the FlightDataExporter import and the deprecated import from tools, appears to be unrelated to the stated purpose of this PR (adding axial acceleration calculation).
These changes seem to be reverting the refactoring that moved export methods to FlightDataExporter, as the methods export_pressures, export_data, export_sensor_data, and export_kml have been restored to the Flight class with their full implementations (lines 3276-3539).
This significantly expands the scope of the PR beyond what is described and should be addressed in a separate PR or the PR description should be updated to reflect these additional changes.
| from ..tools import ( | ||
| calculate_cubic_hermite_coefficients, | ||
| deprecated, | ||
| euler313_to_quaternions, |
There was a problem hiding this comment.
The removal of the deprecated import and restoration of export methods (export_pressures, export_data, export_sensor_data, export_kml) appears to be reverting a previous deprecation without explanation. These methods were previously marked as deprecated in favor of FlightDataExporter class, but are now being restored with full implementations.
This change is unrelated to the PR's stated purpose of adding axial acceleration and represents a significant architectural change that should be discussed separately or properly documented in the PR description.
| @cached_property | ||
| def axial_acceleration(self): | ||
| """Axial acceleration magnitude as a function of time.""" | ||
| return ( | ||
| self.ax * self.attitude_vector_x | ||
| + self.ay * self.attitude_vector_y | ||
| + self.az * self.attitude_vector_z | ||
| ) |
There was a problem hiding this comment.
The axial_acceleration property is missing the @funcify_method decorator that other similar properties use. This decorator is used throughout the codebase to define the output labels and interpolation methods for Function objects. It should include appropriate labels with units.
Suggested fix:
@funcify_method("Time (s)", "Axial Acceleration (m/s²)", "spline", "zero")
def axial_acceleration(self):
"""Axial acceleration magnitude as a function of time, in m/s²."""
return (
self.ax * self.attitude_vector_x
+ self.ay * self.attitude_vector_y
+ self.az * self.attitude_vector_z
)| """Maximum acceleration reached by the rocket.""" | ||
| max_acceleration_time_index = np.argmax(self.acceleration[:, 1]) | ||
| return self.acceleration[max_acceleration_time_index, 1] | ||
|
|
There was a problem hiding this comment.
Trailing whitespace detected. This line should end immediately after the closing brace without extra spaces.
Gui-FernandesBR
force-pushed
the
env/flight-axial_aceleration
branch
from
c5a1258 to
47e5b61
Compare
|
@hogatata are you still willing to work on this one? Tests are not passing, we need that fixed before starting our review |
2 participants
Pull request type
Checklist
pytest tests -m slow --runslow) have passed locallyCurrent behavior
Currently, the Flight class calculates various kinematic properties, but it does not explicitly expose the axial acceleration (acceleration along the rocket's longitudinal axis) as a direct property or method. Users have to calculate it manually from other components.
New behavior
This PR adds the calculation of the axial acceleration to the Flight class:
Breaking change