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.

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

[Bug Report] Lunar Lander Runs Slowly #170

Closed
1 task done
ian-hendo opened this issue Dec 1, 2022 · 10 comments · Fixed by #235
Closed
1 task done

[Bug Report] Lunar Lander Runs Slowly #170

ian-hendo opened this issue Dec 1, 2022 · 10 comments · Fixed by #235
Labels
bug Something isn't working

Comments

@ian-hendo
Copy link

Describe the bug

The Lunar Lander environment creates particles and updates their physics, even when not being rendered. This slows down wall-time training on the environment by ~5x-10x.

Wrapping said code within the step(.) function with if self.render_mode == "human": resolves the issue.

Code example

if self.render_mode == "human":
    p = self._create_particle(
        3.5,  # 3.5 is here to make particle speed adequate
        impulse_pos[0],
        impulse_pos[1],
        m_power,
    )  # particles are just a decoration
    p.ApplyLinearImpulse(
        (ox * MAIN_ENGINE_POWER * m_power, oy * MAIN_ENGINE_POWER * m_power),
        impulse_pos,
        True,
    )

System info

N/A

Additional context

N/A

Checklist

  • I have checked that there is no similar issue in the repo
@ian-hendo ian-hendo added the bug Something isn't working label Dec 1, 2022
@pseudo-rnd-thoughts
Copy link
Member

Wow, if this is true that would be a significant speed up. Could you make a PR with the results and show that the observations don't change as a well

@ian-hendo
Copy link
Author

ian-hendo commented Dec 1, 2022 via email

@pseudo-rnd-thoughts
Copy link
Member

pseudo-rnd-thoughts commented Dec 1, 2022

Yes, I'm at the Uni of Southampton. I will send you an email to your soton address

@mbertheau
Copy link

One issue seems to be that replays won't include particles, if they are only added in render_mode "human".

@pseudo-rnd-thoughts
Copy link
Member

We can include the optimising in the render_mode=="rgb_array"

@mbertheau
Copy link

I'm currently at if self.render_mode is not None. That seems to cover all cases.

@pseudo-rnd-thoughts
Copy link
Member

Would someone make PR with this fix along with a test to show that this doesn't break anything

@mbertheau
Copy link

What did you have in mind for the test?

@pseudo-rnd-thoughts
Copy link
Member

If the rendering is deterministic, could you save the rendered frames then make the change and see if the rendered frames are the same (using the same actions and reset seed). Don't think we want this added to the actual tests, could include the test code for someone to replicate.
Then add a quick performance test to show the difference in fps?

I don't think we can add anything to the actual tests in the project as the old version won't exist after the fix.

@PaulMest
Copy link
Contributor

PaulMest commented Jan 2, 2023

I've been going through the HuggingFace RL course over the holidays and also discovered this. Submitted the fix for it here: #235. Results appear to be pretty great for such a small change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants