[#225] Calculate orbital period for observation norms - #353
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0bec8934d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """Minimum time before the completion bonus may be awarded.""" | ||
| if self.min_time_for_completion is not None: | ||
| return self.min_time_for_completion | ||
| return self.scenario.satellites[0].dynamics.orbital_period |
There was a problem hiding this comment.
Use the RSO's period for the completion threshold
When callers list an inspector before the RSO, this selects the inspector's orbital period even though RSOPoints.reset_pre_sim_init() explicitly identifies the unique RSO independently of satellite order. Because inspectors can maneuver and occupy a different orbit, the default completion threshold can then shift or use the wrong period; read self.scenario.rso.dynamics.orbital_period instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Calculates satellite-specific orbital periods from Keplerian elements and uses them for observation normalization and RSO completion timing.
Changes:
- Added orbital-period utilities and dynamics support.
- Updated observation, relative-observation, and RSO defaults.
- Updated tests, examples, benchmarks, and release notes.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tests/unittest/utils/test_orbital.py |
Tests orbital-period calculations. |
tests/unittest/sim/test_dynamics.py |
Tests the dynamics orbital-period property. |
tests/unittest/obs/test_relative_observations.py |
Tests relative-observation normalization. |
tests/unittest/obs/test_observations.py |
Tests observation normalization. |
tests/unittest/data/test_rso_inspection_data.py |
Tests RSO completion timing. |
src/bsk_rl/utils/orbital.py |
Adds orbital-period calculation. |
src/bsk_rl/sim/dyn/base.py |
Exposes orbital period on dynamics. |
src/bsk_rl/obs/relative_observations.py |
Supports period-based normalization. |
src/bsk_rl/obs/observations.py |
Uses orbital periods for normalization. |
src/bsk_rl/data/rso_inspection.py |
Uses orbital period for completion timing. |
examples/training_with_shield.ipynb |
Updates example period usage. |
examples/time_discounted_gae.ipynb |
Updates example period usage. |
examples/satellite_configuration.ipynb |
Updates example period usage. |
examples/rso_inspection.ipynb |
Updates example period usage. |
examples/rllib_training.ipynb |
Updates example period usage. |
examples/multiagent_envs.ipynb |
Updates example period usage. |
examples/fault_environment.ipynb |
Updates example period usage. |
examples/curriculum_learning.ipynb |
Updates example period usage. |
examples/communication_action.ipynb |
Updates example period usage. |
examples/cloud_environment.ipynb |
Updates example period usage. |
examples/cloud_environment_with_reimaging.ipynb |
Updates example period usage. |
examples/async_multiagent_training.ipynb |
Updates example period usage. |
examples/aeos.ipynb |
Updates example period usage. |
docs/source/release_notes.rst |
Documents the feature. |
benchmarks/rso_inspection.py |
Updates RSO benchmark timing. |
benchmarks/nadir_science.py |
Uses calculated orbital periods. |
benchmarks/aeos.py |
Uses calculated orbital periods. |
Suppressed comments (1)
src/bsk_rl/data/rso_inspection.py:223
- This selects the first satellite in the scenario rather than the RSO identified by
RSOPoints.rso. If an inspector or another satellite is listed first, the default completion delay is based on that satellite's orbit, so it can be the wrong duration. Use the scenario'srsosatellite for this default (the scenario already sets it inreset_pre_sim_init).
return self.scenario.satellites[0].dynamics.orbital_period
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Description
Closes #225
Replaces the hardcoded 5700 s (~1 LEO orbit) used for observation time normalization with an orbital period calculated from the satellite's Keplerian elements.
obs.Eclipse()now defaults to this, andnorm=Nonedoes the same for other observation properties.RSOInspectionReward.min_time_for_completionalso defaults to one orbital period.Type of change
How should this pull request be reviewed?
How Has This Been Tested?
Unit tests for orbital period, observation norms, and RSO completion timing. Integration tests for observations and the RSO environment.
Future Work
A few
time_limit/max_drift_durationvalues are still 5700 where the orbit is not known at env construction.Checklist