Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature/misc improvements #190

Merged
merged 7 commits into from
Sep 23, 2024
Merged

Feature/misc improvements #190

merged 7 commits into from
Sep 23, 2024

Conversation

Mark2000
Copy link
Contributor

@Mark2000 Mark2000 commented Sep 18, 2024 •
edited
Loading

Description

Closes #189
Closes #185
Closes #188
Closes #187
Closes #191

Misc improvements. Review by commit.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How should this pull request be reviewed?

  • By commit
  • All changes at once

How Has This Been Tested?

Tests updated.

Passes Tests

  • Unit tests pytest --cov bsk_rl --cov-report term-missing tests/unittest
  • Integrated tests pytest --cov bsk_rl --cov-report term-missing tests/integration
  • Documentation builds cd docs; make html

Test Configuration

  • Python: 3.10
  • Basilisk: 2.4.17
  • Platform: MacOS

Checklist:

  • My code follows the style guidelines of this project (passes Black, ruff, and isort)
  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation and release notes
  • Commit messages are atomic, are in the form Issue #XXX: Message and have a useful message
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • If I changed an example ipynb, I have locally rebuilt the documentation

@Mark2000 Mark2000 changed the base branch from develop to feature/marl September 18, 2024 22:56
@Mark2000 Mark2000 force-pushed the feature/misc-improvements branch from 98eea1a to 418882a Compare September 18, 2024 23:32
LorenzzoQM
LorenzzoQM reviewed Sep 19, 2024
View reviewed changes
src/bsk_rl/gym.py
agents = [
satellite.name
for satellite in self.satellites
if (satellite.is_alive() and not truncated)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truncated is just one boolean for the episode and not for each satellite, right? I guess you could just check if it is truncated before iterating over the satellites to make it faster? Still, I am not sure if this has any practical advantages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was copying some example from petting zoo. I don’t see that being a huge performance difference.

LorenzzoQM
LorenzzoQM reviewed Sep 19, 2024
View reviewed changes
src/bsk_rl/gym.py
@@ -306,7 +311,18 @@ def _get_obs(self) -> MultiSatObs:
Returns:
tuple: Joint observation
"""
return tuple(satellite.get_obs() for satellite in self.satellites)
if self.generate_obs_retasking_only:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

LorenzzoQM
LorenzzoQM reviewed Sep 19, 2024
View reviewed changes
docs/source/release_notes.rst
@@ -19,6 +19,7 @@ Development Version
performance.
* Add option to ``generate_obs_retasking_only`` to prevent computing observations for
satellites that are continuing their current action.
* Improve performance of :class:`~bsk_rl.obs.Eclipse` observations by about 95%.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent change!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add the change in the access filters here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll note that it's possible to restrict access filters to only act on a certain type of opportunity. The base filter behavior should be the same.

@Mark2000 Mark2000 force-pushed the feature/misc-improvements branch from 75b7fce to c3901d3 Compare September 19, 2024 20:42
@Mark2000 Mark2000 linked an issue Sep 19, 2024 that may be closed by this pull request
Eclipse observation is too slow #191
Closed
@Mark2000 Mark2000 force-pushed the feature/misc-improvements branch from c3901d3 to 7a05cbb Compare September 20, 2024 17:30
Base automatically changed from feature/marl to develop September 23, 2024 17:16
@Mark2000 Mark2000 force-pushed the feature/misc-improvements branch from 7a05cbb to b67c312 Compare September 23, 2024 17:31
@Mark2000 Mark2000 force-pushed the feature/misc-improvements branch from b67c312 to 69d2125 Compare September 23, 2024 17:34
@Mark2000 Mark2000 requested a review from LorenzzoQM September 23, 2024 17:34
@Mark2000
Copy link
Contributor Author

Added a docstring for a method that was missing one

LorenzzoQM
LorenzzoQM approved these changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@LorenzzoQM LorenzzoQM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good changes and excellent performance improvement!

@Mark2000 Mark2000 merged commit 011f851 into develop Sep 23, 2024
4 checks passed
@Mark2000 Mark2000 deleted the feature/misc-improvements branch September 23, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LorenzzoQM LorenzzoQM LorenzzoQM approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@Mark2000 @LorenzzoQM