Conversation
3d6dafa to
239b9ee
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the inheritance structure of the dynamics and flight software models to support flexible composition through multiple inheritance. Users can now specify dyn_type and fsw_type as tuples of classes rather than manually creating new classes.
Key Changes:
- Introduced base classes
BaseDynamicsModel,BaseFSWModel,BaseWorldModelwith minimal functionality, allowing more granular composition - Added
compose_types()utility to handle dynamic type composition with MRO resolution - Removed
world_typeparameter requirement from test cases (now auto-inferred) - Updated world model setup to use more modular inheritance with
EclipseWorldModelandAtmosphereWorldModel
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/bsk_rl/utils/functional.py | Added type composition utilities (compose_types, remove_duplicate_bases, etc.) and imported MagicMock |
| src/bsk_rl/sats/satellite.py | Added get_dyn_type() and get_fsw_type() methods to compose types from tuples |
| src/bsk_rl/sim/dyn/base.py | Split BasicDynamicsModel into BaseDynamicsModel (minimal) and BasicDynamicsModel (full-featured) |
| src/bsk_rl/sim/fsw/base.py | Moved Task class definition before FSWModel and created BaseFSWModel |
| src/bsk_rl/sim/world.py | Renamed BasicWorldModel to BaseWorldModel and split into modular components |
| src/bsk_rl/gym.py | Updated _minimum_world_model() to use compose_types() |
| tests/unittest/*.py | Removed world_type=MagicMock() from test instantiations |
| examples/*.ipynb | Replaced types.new_class() with tuple syntax for type composition |
Comments suppressed due to low confidence (2)
src/bsk_rl/sim/dyn/base.py:1
- A new attribute
min_orbital_radiusis being set in the test, but this attribute is now a parameter insetup_spacecraft_hub()with a default value. The test should verify that this attribute is properly set during initialization rather than manually setting it, or document why manual setting is necessary for this test.
"""Basic dynamics model for BSK-RL."""
tests/unittest/sats/test_satellite.py:1
- The new line adds
dyn_type=MagicMock()to theSatellitepatch decorator, but this is immediately overwritten bysats.Satellite.dyn_type = MockDynTypeon line 45. This creates confusion about which value is actually used. Either remove the duplicate assignment or add a comment explaining why both are needed.
from functools import partial
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ad895e3 to
df698a3
Compare
|
TODO: Should document dynamics and fsw selection more explicitly somewhere |
fee7375 to
866a8c9
Compare
|
TODO: make _create_module_data not abstract |
866a8c9 to
e981945
Compare
12d772c to
d3b5539
Compare
d3b5539 to
c9bdaa2
Compare
6600e93 to
1b7883e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Part of #156
See copilot summary.
Type of change
How should this pull request be reviewed?
How Has This Been Tested?
Please describe how tests have been updated to verify your changes.
Future Work
What future tasks are needed, if any?
Checklist