solved environment parameter global usage, now can be used locally. #854
Conversation
|
@MateusStano could you review this one please? |
There was a problem hiding this comment.
Pull Request Overview
This PR solves the issue where air brake controllers relied on global environment variables by adding an environment parameter to the controller function signature. This enables local access to atmospheric conditions without using global variables.
- Added environment parameter support to controller functions (6, 7, or 8 parameters now supported)
- Updated controller initialization to handle the new environment parameter with backward compatibility
- Created test fixtures and standalone test script to demonstrate the new functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures/function/function_fixtures.py | Added new test fixture demonstrating environment parameter usage in controller |
| test_environment_parameter.py | Standalone test script showing different controller signatures and environment usage |
| rocketpy/simulation/flight.py | Updated simulation to pass environment parameter to controllers |
| rocketpy/rocket/rocket.py | Updated documentation to describe the new environment parameter |
| rocketpy/control/controller.py | Enhanced controller initialization to support 8-parameter functions and updated call signature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| altitude_ASL = state[2] # altitude above sea level | ||
| altitude_AGL = altitude_ASL - environment.elevation # altitude above ground level | ||
| vx, vy, vz = state[3], state[4], state[5] | ||
|
|
There was a problem hiding this comment.
Remove trailing whitespace on line 147 to maintain code cleanliness.
Copilot uses AI. Check for mistakes.
| print(" • Backward compatibility maintained") | ||
|
|
||
| print(f"\n🚀 Example usage in controller:") | ||
| print(" # Old way (with global variables):") |
There was a problem hiding this comment.
[nitpick] The example shows 'env' as a global variable, but this is misleading. The old way would typically use a different pattern. Consider showing a more accurate example of how global variables were actually accessed before this change.
| print(" # Old way (with global variables):") | |
| print(" # Old way (with global variables):") | |
| print(" from globals import env # ❌ Import global variable") |
Copilot uses AI. Check for mistakes.
| "observed_variables, interactive_objects, sensors, environment). " | ||
| "The last two arguments (sensors and environment) are optional." |
There was a problem hiding this comment.
The error message is inconsistent - it states that both sensors and environment are optional, but a 6-parameter function doesn't include sensors. Consider clarifying that sensors is optional for 6-7 parameter functions, and environment is optional for 6-8 parameter functions.
| "observed_variables, interactive_objects, sensors, environment). " | |
| "The last two arguments (sensors and environment) are optional." | |
| "observed_variables, interactive_objects[, sensors[, environment]]). " | |
| "The 'sensors' argument is optional for 6-7 parameter functions, " | |
| "and the 'environment' argument is optional for 6-8 parameter functions." |
Copilot uses AI. Check for mistakes.
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest tests -m slow --runslow) have passed locallyCurrent behavior
here, air brakes controller doesn't pass environment as parameter but uses the global variables.
New behavior
Now, air brakes controller uses env parameter, you can use the file "test_environment_parameter.py" to check functioning.
Breaking change
Additional information
Enter text here...