-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Relative Motion #214
base: develop
Are you sure you want to change the base?
Relative Motion #214
Conversation
56221bc
to
74cf1de
Compare
e9937e2
to
841d9fb
Compare
841d9fb
to
f686acf
Compare
31aecce
to
e1ca657
Compare
src/bsk_rl/utils/orbital.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this random unit vector used for?
src/bsk_rl/act/continuous_action.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the hardcoded '5700.0' specify here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened a new issue to track this, since we use hardcoded 5700 as a normalization factor (approx. 1 orbit) in various places. #225
src/bsk_rl/sim/fsw.py
Outdated
@@ -1008,15 +1032,17 @@ def action_magic_thrust(self, dv_N: np.ndarray) -> None: | |||
Args: | |||
dv_N: [m/s] Inertial Delta V. | |||
""" | |||
if np.linalg.norm(dv_N) > self.dv_available: | |||
self.satellite.logger.warning( | |||
f"Maneuver exceeds available Delta V ({np.linalg.norm(dv_N)}/{self.fuel_remaining} m/s)." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this give you the percentage (above 100%) that the current requested dv maneuver exceeds the available amount by? This division shouldn't give m/s unless I am missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The print statement results in something like Maneuver exceeds available Delta V (3.5/1.2 m/s).
f"Maneuver exceeds available Delta V ({np.linalg.norm(dv_N)}/{self.fuel_remaining} m/s)." | ||
) | ||
|
||
self._dv_available -= np.linalg.norm(dv_N) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if np.linalg.norm(dv_N) > self.dv_available then it would not be subtracted from the _dv_available since it is not a valid/possible maneuver. I think if np.linalg.norm(dv_N) > self.dv_available then we need to pass and step out of the action_magic_thrust function right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, definitely should handle this more gracefully. Could just perform a dv_available
sized thrust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left comments on some of the commits just to clarify... I don't thing they are necessarily things you need to change except in the location where it is checked if np.linalg.norm(dv_N) > self.dv_available and even if that is the case we reduce the amount of _dv_available. We should fire the maneuver if it exceeds are dv capability so we would not reduce future dv_availability.
Also I saw that the build gives some issues (like the python 3.11 or 3.10) but they dont seem to be relevant issues right.
Description
Closes #144
Please include a summary, motivation, and context of the changes and the related issue.
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