[#337] Allow continuous aliveness checkers - #354
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbb3bf7288
ℹ️ 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".
| and not is_property(model, name) | ||
| and callable(getattr(model, name)) |
There was a problem hiding this comment.
Because self.dynamics and self.fsw are weak-reference proxies, is_property(model, name) examines weakref.ProxyType rather than the underlying model class and therefore returns false for model properties. The following getattr calls consequently execute every property getter during simulator construction, even when no continuous checker exists; a custom property that requires initialized simulation state or deliberately raises will now make env.reset() fail. Inspect the underlying class (for example via model.__class__) before retrieving attributes.
Useful? React with 👍 / 👎.
| valid_func_name( | ||
| f"aliveness_{self.name}_{type(model).__name__}_{name}" | ||
| ), |
There was a problem hiding this comment.
When both the dynamics and FSW models define a continuous checker with the same method name, these registrations produce the same event name: model is a weak-reference proxy in both iterations, so type(model).__name__ is ProxyType, not the dynamics or FSW class name. The second registration therefore reuses the first event-map key, preventing both checkers from remaining independently active; use the underlying class name or an explicit dynamics/FSW discriminator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds support for “continuous” aliveness checkers so selected @aliveness_checker methods can be evaluated via Basilisk events during integration and immediately interrupt an environment step upon failure, while preserving existing step-level checker behavior.
Changes:
- Extend
@aliveness_checkerto acceptcontinuousandcheck_rateoptions and expose them on the wrapped checker. - Create satellite-level Basilisk events for continuous checkers during simulator initialization.
- Add unit + integration tests validating decorator metadata and step-level vs continuous interruption behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittest/utils/test_functional.py | Adds unit tests for new decorator defaults and kwargs (continuous, check_rate). |
| tests/unittest/sats/test_satellite.py | Adds unit test verifying continuous checkers create terminal Basilisk events and mark the satellite dead. |
| tests/integration/sim/test_int_dynamics.py | Adds integration test comparing step-level vs continuous termination time. |
| src/bsk_rl/utils/functional.py | Updates aliveness_checker decorator to support optional configuration and store metadata on wrapped callables. |
| src/bsk_rl/sim/simulator.py | Hooks satellite continuous aliveness event setup into simulator initialization. |
| src/bsk_rl/sim/fsw/init.py | Documents the new continuous=True behavior for FSW checkers. |
| src/bsk_rl/sim/dyn/init.py | Documents the new continuous=True behavior for dynamics checkers. |
| src/bsk_rl/sats/satellite.py | Implements setup_aliveness_events() to create terminal Basilisk events for continuous checkers. |
| docs/source/release_notes.rst | Adds release note entry for continuous aliveness checkers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| self.simulator.createNewEvent( | ||
| valid_func_name( | ||
| f"aliveness_{self.name}_{type(model).__name__}_{name}" |
| def side_effect(sim, checker=checker): | ||
| checker(log_failure=True) | ||
| self._is_alive = False | ||
| self.record_death(sim.sim_time) |
Description
Closes #337
Adds a
continuousoption to@aliveness_checkerso checkers can run as Basilisk events and stop the environment step when they fail. Existing checkers are unchanged.Type of change
How should this pull request be reviewed?
How Has This Been Tested?
Unit tests for the decorator and event setup. Integration test comparing step-level vs continuous interruption.
Future Work
Built-in checkers (battery, altitude, RW speed, etc.) still only run at environment steps.
Checklist