Repository navigation
Make simulation time step configurable end to end (#8) - #97
Open
pranavvadd wants to merge 5 commits into
Open
pranavvadd wants to merge 5 commits into
pranavvadd wants to merge 5 commits into
Conversation
TrackConfig gained a required file_path_name field in #60; the test hit a TypeError before reaching the lap-distance validation it checks. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Fix test_invalid_track_config for required file_path_name
Weather data was always generated at 1-minute spacing, so any time_step_minutes other than 1 produced a wrong time axis and wrong distance/SoC. Resample weather onto the configured step (averaging for coarser steps, interpolating for finer) and expose --time-step-minutes on the CLI for single runs, --plan and --speed-sweep. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Contributor
|
yeah I merged the other one first, so small change for that and another small bug fix, the commits u make on the branch will show up here so we can marge once its handled: Overall looks good! The test coverage is especially thorough. One edge case I noticed in Could we handle the final partial step and add a test to verify the simulated race duration stays correct for non-divisible time steps? Otherwise, good |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the simulation time step work end to end.
RaceConfig.time_step_minutesalreadyexisted, but weather data was always generated at 1-minute spacing, so any other step
gave a wrong time axis and wrong distance/SoC. Example: with a 5-minute step the race
ended after about 29 minutes of race time (should be about 160) and covered 142 km
instead of 196 km.
Changes
sim/weather.py: newresample_to_time_step(), applied to both synthetic and APIweather. Steps over 1 minute average the data (preserves solar energy), steps under
1 minute interpolate, and a 1-minute step is unchanged.
sim/main.py: new--time-step-minutesoption (default 1.0, must be a finite numberabove 0). It replaces the three hardcoded
1.0values and works for single runs,--planand--speed-sweep.README.md: documents the flag.tests/test_time_step.py: 158 new pytest tests.Testing
python3 -m pytest tests/test_time_step.pypasses. After the fix, distance is 196.2-196.4 kmat every step from 0.5 to 10 minutes. The tests also fail (48 failures) if the old behavior is
restored. They cover resampling, synthetic and API weather, the simulator against a
1-minute baseline, all four strategies,
interval-hold, and the CLI.Related Issue
Closes #8
Notes
pi,stepped,interval-hold) react per step, so coarse stepsaren't identical to the 1-minute run. The tests allow up to 10% distance difference.
--no-api-weather) also changessim/main.py, in different lines. Whichever of thetwo PRs merges second may need a small conflict fix.