Skip to content

Conversation

@jjshoots
Copy link
Contributor

@jjshoots jjshoots commented Apr 11, 2022

Description

This change is for LunarLander.
This adds adjustable gravity and wind to the env.
More changes to this env and perhaps other envs are possible in due time.

To set gravity value to say, 5, simply pass gravity=-5.

To enable wind, pass the wind kwarg enable_wind=True.
By default, the wind function is C tanh(sin(2 k x) + sin(pi k x), which is proven to be non-periodic for all k (see this).
In implementation, I've set k = 0.01, C=15.
This is just a simpler version instead of using a Dryden wind field or Perlin noise sampling.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@jjshoots jjshoots changed the title Fix impulse sample bug and add gravity as kwarg in Lunar Lander Add gravity as kwarg in Lunar Lander Apr 11, 2022
@balisujohn
Copy link
Contributor

This is interesting; I could definitely see this kind of parameterization being useful for sim2sim research.

@jjshoots jjshoots changed the title Add gravity as kwarg in Lunar Lander Add gravity and wind as kwarg in Lunar Lander Apr 12, 2022
def demo_heuristic_lander(env, seed=None, render=False):

# wind power must be reduced for heuristic landing
global WIND_POWER
Copy link
Contributor

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?

Copy link
Contributor Author

@jjshoots jjshoots Apr 13, 2022

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.

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've made it a kwarg in the latest commit.

Copy link
Collaborator

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jkterry1 done!

@RedTachyon
Copy link
Contributor

So actually, what is the rationale for keeping the wind strength a constant? It might be a good option to showcase curriculum learning by changing its value throughout the training (I'm assuming learning is easier if there is no wind, and more difficult if it's stronger)

It might even be an opportunity to showcase the new reset optional arguments and make its magnitude settable at reset.

@jkterry1 what do you think?

@jkterry1 jkterry1 merged commit 681a992 into openai:master Apr 15, 2022
"LunarLander-v2",
continuous: bool = False,
gravity: float = -10.0,
enable_wind: bool = False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should the env be renamed into "Lander"? LunarLander makes it sound like the landing is happening on the moon, in which case, accounting for wind does not seem necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

While that could be more accurate to change the name, for posterity's sake (and breaking a load of code) then we won't change the name

"LunarLander-v2",
continuous: bool = False,
gravity: float = -10.0,
enable_wind: bool = False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should the env be renamed into "Lander"? LunarLander makes it sound like the landing is happening on the moon, in which case, accounting for wind does not seem necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants