-
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
Issue #136: Speed up root finding #137
Conversation
Going to add some additional changes in a second commit, but won't affect the first commit |
@@ -294,7 +294,7 @@ class AccessSatellite(Satellite): | |||
def __init__( | |||
self, | |||
*args, | |||
generation_duration: float = 60 * 95, | |||
generation_duration: float = 600.0, |
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.
Should it be 6,000.0 instead of 600.0 to be closer to 60*95?
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.
No, 6000 is excessive. Usually what happens is you calculate up to your time limit, but at the end you just need a handful of extra observations past the time limit to fill out your observation. If you needed to go past 600s, it would calculate additional 600s increments until it has everything it needs.
Added a second commit that improves some bounds on a search by calculating them off the fly. Leads to a >80% reduction in reset time in my scenario. |
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.
Looks good! Also, it is good not having to install chebpy considering the compatibility issues we were having.
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.
Works good with my script.
Description
Closes #136
Gets rid of chebpy and instead uses our knowledge about the problem to solve it more efficiently.
Before: 0.728 ms/call
After: 0.318 ms/call
Effective reduction of
reset()
times by 0.5x in imaging tasks.Type of change
How should this pull request be reviewed?
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Passes Tests
pytest --cov bsk_rl/envs/general_satellite_tasking --cov-report term-missing tests/unittest
pytest --cov bsk_rl/envs/general_satellite_tasking --cov-report term-missing tests/integration
pytest tests/examples
Test Configuration
Checklist:
Issue #XXX: Message
and have a useful message