-
-
Notifications
You must be signed in to change notification settings - Fork 1
DEV: custom atmosphere schema changes #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
""" WalkthroughThe changes extend the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EnvironmentModel
participant EnvironmentService
participant RocketPyEnvironment
User->>EnvironmentModel: Provide environment data (including pressure, temperature, wind_u, wind_v)
EnvironmentModel->>EnvironmentService: Pass model instance
EnvironmentService->>RocketPyEnvironment: set_atmospheric_model(type, file, pressure, temperature, wind_u, wind_v)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the environment schema to support custom atmospheric inputs and standardizes the height type.
- Changed
max_expected_heightfrominttofloatin the simulation view - Extended the
EnvironmentModelto includepressure,temperature,wind_u, andwind_vfields - Passed the new parameters into the
rocketpy_env.set_atmospheric_modelcall
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/views/environment.py | Updated max_expected_height annotation from int to float |
| src/models/environment.py | Added pressure, temperature, wind_u, and wind_v fields |
| src/services/environment.py | Propagated new atmospheric parameters into model initializer |
Comments suppressed due to low confidence (3)
src/services/environment.py:34
- There are no tests covering the new custom atmospheric parameters (
pressure,temperature,wind_u,wind_v) being passed intoset_atmospheric_model. Consider adding unit tests for these new flows.
pressure=env.pressure,
src/views/environment.py:25
- Changing
max_expected_heightfrominttofloatmay introduce type inconsistencies in downstream logic; ensure all consumers handle float values properly or update related validation and documentation.
max_expected_height: Optional[float] = None
src/services/environment.py:34
- Consider validating or providing default handling for
env.pressure(and the other new parameters) before passing them toset_atmospheric_modelto avoid unexpectedNonevalues at runtime.
pressure=env.pressure,
src/models/environment.py
Outdated
| pressure: Optional[float | List[Tuple[float, float]]] = None | ||
| temperature: Optional[float | List[Tuple[float, float]]] = None | ||
| wind_u: Optional[float | List[Tuple[float, float]]] = None | ||
| wind_v: Optional[float | List[Tuple[float, float]]] = None |
Copilot
AI
Jul 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The union type annotation using typing.List and typing.Tuple can be simplified with built-in generics (e.g., list[tuple[float, float]]) for consistency with modern Python syntax.
| pressure: Optional[float | List[Tuple[float, float]]] = None | |
| temperature: Optional[float | List[Tuple[float, float]]] = None | |
| wind_u: Optional[float | List[Tuple[float, float]]] = None | |
| wind_v: Optional[float | List[Tuple[float, float]]] = None | |
| pressure: Optional[float | list[tuple[float, float]]] = None | |
| temperature: Optional[float | list[tuple[float, float]]] = None | |
| wind_u: Optional[float | list[tuple[float, float]]] = None | |
| wind_v: Optional[float | list[tuple[float, float]]] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its totally a minor but I dont see why to stick with the old hinting since we keep python very updated for infinity.
Wdyt @aasitvora99 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't realize this was a thing in Python. fixed with 8854300
GabrielBarberini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just make sure to make format, make lint and make test then merge it
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
deda92e
Fixes #54
Summary by CodeRabbit
New Features
Improvements