Address PR #245 review: add documentation, fix initialization bug, expand test coverage#246
Merged
Merged
Conversation
- Add comprehensive documentation for ExecutionLimits struct - Add comprehensive documentation for ExecutionMonitor struct and methods - Fix incorrect arguments to initialize_ukf at line 419 in sim/src/main.rs - Add comment explaining unused execution_limits in geo navigation - Add test coverage for ExecutionMonitor (wall-clock, success case, edge cases) - Fix type mismatch bug for last_ts variable in run_closed_loop - Update integration tests to pass execution_limits parameter Co-authored-by: jbrodovsky <57160841+jbrodovsky@users.noreply.github.com>
…arguments Co-authored-by: jbrodovsky <57160841+jbrodovsky@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add execution timeouts for simulations
Address PR #245 review: add documentation, fix initialization bug, expand test coverage
Jan 20, 2026
jbrodovsky
marked this pull request as ready for review
January 20, 2026 15:25
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses review feedback from PR #245 by adding comprehensive documentation for execution timeout monitoring, fixing initialization bugs, and significantly expanding test coverage. The changes improve code quality, correctness, and maintainability without introducing any issues.
Changes:
- Added detailed documentation for
ExecutionLimitsandExecutionMonitorwith usage examples - Fixed
initialize_ukfcall to useUkfConfig::default()instead of incorrect parameter passing - Fixed type mismatch in
last_tsvariable initialization (Option<DateTime<Utc>>vsDateTime<Utc>) - Expanded test coverage from 1 to 5 test cases covering multiple timeout scenarios
- Removed unused
mutqualifiers and added appropriate clippy attributes
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| core/src/sim.rs | Added comprehensive documentation for ExecutionLimits and ExecutionMonitor; fixed last_ts type to Option<DateTime>; added 4 new execution monitor tests |
| sim/src/main.rs | Fixed initialize_ukf call to use UkfConfig::default(); removed unused mut qualifiers; added clippy attribute for helper function; added clarifying comment about execution limits in geophysical navigation |
| core/tests/integration_tests.rs | Updated run_closed_loop calls to include new execution_limits parameter (set to None) |
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
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.
Addresses review feedback on execution timeout monitoring implementation. The original changes lacked documentation, had an incorrect filter initialization call, and insufficient test coverage.
Documentation
ExecutionLimitsandExecutionMonitorexplaining timeout mechanisms (wall-clock ratio, absolute wall-clock, no-progress detection)ExecutionMonitor::check()doc commentBug Fixes
initialize_ukfcall: Changed from passing 7 individualNoneparameters toUkfConfig::default()last_tsvariable initialization inrun_closed_loop(wasDateTime<Utc>, neededOption<DateTime<Utc>>)Test Coverage
Expanded
ExecutionMonitortests from 1 to 5 cases:Code Quality
mutqualifiers flagged by clippy#[allow(clippy::too_many_arguments)]for helper function with legitimate parameter count💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.