fix(environment): default missing wind to 0 for custom_atmosphere#74
fix(environment): default missing wind to 0 for custom_atmosphere#74Gui-FernandesBR wants to merge 1 commit into
Conversation
RocketPy's set_atmospheric_model guards pressure/temperature against None but not wind, so a custom_atmosphere environment with wind_u/wind_v left unset raised "'NoneType' object has no attribute 'shape'" and hard-failed the simulation. Default a None wind component to 0.0 (only None is replaced — real 0.0 values and wind-profile lists pass through untouched). Surfaced from the Jarvis beta audit: the web client seeds wind as null and only writes on user edit, so the default Custom-atmosphere path crashed. Jarvis now coerces null->0 on egress too; this is the belt-and-suspenders backend guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesEnvironment wind defaults
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/services/environment.py (2)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the comment to stay within 79 characters.
Line 34 exceeds the repository’s configured line-length limit. As per coding guidelines:
**/*.py: Target Python 3.12 and enforce 79-character line length using Black, Ruff, and Pylint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/environment.py` at line 34, Wrap the comment near the environment value handling so every line is no longer than 79 characters, preserving its meaning that only None is replaced while real 0.0 values and wind profiles pass through.Source: Coding guidelines
35-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the normalization contract.
Cover both components being unset, only one component being unset, explicit
0.0, and profile lists. This verifies that onlyNoneis replaced and that existing wind inputs remain unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/environment.py` around lines 35 - 41, Add regression tests for the atmospheric model setup in the environment service, covering both wind components unset, either component unset individually, explicit 0.0 values, and profile-list inputs. Assert that only None values normalize to 0.0 and all existing wind inputs remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/services/environment.py`:
- Line 34: Wrap the comment near the environment value handling so every line is
no longer than 79 characters, preserving its meaning that only None is replaced
while real 0.0 values and wind profiles pass through.
- Around line 35-41: Add regression tests for the atmospheric model setup in the
environment service, covering both wind components unset, either component unset
individually, explicit 0.0 values, and profile-list inputs. Assert that only
None values normalize to 0.0 and all existing wind inputs remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 08c092ef-94bd-48ce-8458-ecc8942284b4
📒 Files selected for processing (1)
src/services/environment.py
Default missing wind to 0 for
custom_atmosphereRocketPy's
set_atmospheric_modelguardspressure/temperatureagainstNonebut not wind. Acustom_atmosphereenvironment withwind_u/wind_vleft unset therefore raisedAttributeError: 'NoneType' object has no attribute 'shape'and hard-failed the simulation.EnvironmentService.from_env_modelnow defaults aNonewind component to0.0. OnlyNoneis replaced — a real0.0and wind-profile lists pass through untouched.Why
Surfaced from the Jarvis beta-prep audit: the web client seeds wind as
nulland only writes on user edit, so the default Custom-atmosphere path crashed the sim (UI showed0while sendingnull). Jarvis has been fixed to coercenull → 0on egress; this is the belt-and-suspenders guard on the backend so any client (or a direct API call) is safe.Test
set_atmospheric_model(type='custom_atmosphere', wind_u=None)reproduced the crash; withwind_u=0.0it succeeds.🤖 Generated with Claude Code
Summary by CodeRabbit