-
Notifications
You must be signed in to change notification settings - Fork 8.7k
Add gravity and wind as kwarg in Lunar Lander #2746
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
Merged
Merged
Changes from 11 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
ded92cc
fix impulse sample bug and add adjustable gravity
jjshoots 793e9ea
Merge branch 'openai:master' into master
jjshoots 6ba1048
revert weird sample thing
jjshoots 736017b
assertation
jjshoots 1b33ba1
add wind
jjshoots b13fbe0
fix wind to use applyforce
jjshoots 1942a7f
black
jjshoots eca1a30
add tests for wind
jjshoots 3b9dbe0
fix test bug
jjshoots 5656bf8
run lunar lander tests over 10 runs
jjshoots b71d790
reduce wind for heuristic landing
jjshoots 6e96aab
made wind power kwarg
jjshoots c2c181f
reduce wind power for heuristic landing
jjshoots 01b348a
update doc
jjshoots 208eaa9
remove wind power from kwarg
jjshoots c156512
fix tests by deleting tests ;)
jjshoots 3f4c59c
Merge branch 'openai:master' into master
jjshoots 2487115
Add back wind power as kwarg
jjshoots 3d3aac7
the black sheep
jjshoots bf0037c
remove rogue plus
jjshoots File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is a big no-no, all caps suggests that this should be a global constant, so changing this in the code is a bad pattern.
If the wind power is something that users are meant to change, maybe make it a constructor argument?
Uh oh!
There was an error while loading. Please reload this page.
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.
Hi Ariel, the wind power isn't something that user's are meant to change.
The only reason it's here is that the heuristic landing won't work with the default wind setting, and this causes it to fail tests. So for heuristic landings, I've reduced the wind power to be 1/15th of the original magnitude (so that the wind code is still tested in tests, but still solvable with heuristic landing).
I'm aware this is not the most optimal solution, I'll think of something soon.
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've made it a kwarg in the latest commit.
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 you want users to not change it (I don't know why that'd be the case, but I trust your judgement), wouldn't the standard be to name it _wind_power and not have it be a kwarg?
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.
@jkterry1 done!