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

CartPole: fix vector rendering (#872) #937

Closed
wants to merge 1 commit into from
Closed

CartPole: fix vector rendering (#872) #937

wants to merge 1 commit into from

Conversation

RogerJL
Copy link
Contributor

@RogerJL RogerJL commented Feb 24, 2024 •
edited
Loading

Also corrected some bugs in vectorized Cartpole

Description

This fixes some, probably breaking, issues in Chartpole

  1. Testing if clocks is not set, created clock if not, resulting in a new clock array created every turn.
  2. Looping over states but assigned x with full all states

Fixes # (872)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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
    Tests redesigned, will arrive in HumanRenderer for Vector environments

@RogerJL
ChartPole: fix vector rendering (Farama-Foundation#872)
2ff8c15 
Also corrected some bugs in vectorized Cartpole
pseudo-rnd-thoughts
pseudo-rnd-thoughts reviewed Feb 27, 2024
View reviewed changes
gymnasium/envs/classic_control/cartpole.py
x = self.state.T

for state, screen, clock, x in zip(
self.state, self.screens, self.clocks, self.state.T
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need both state and state.T? This seems extremely weird.
I'm guessing that one of them is incorrect and should be removed

@Kallinteris-Andreas Kallinteris-Andreas changed the title ChartPole: fix vector rendering (#872) CartPole: fix vector rendering (#872) Feb 27, 2024
@pseudo-rnd-thoughts
Copy link
Member

Testing the PR, it doesn't actually allow human rendering for num_envs>1; therefore, I think the easiest solution is to remove human rendering from vector environments and use the vector.HumanRendering wrapper proposed in #880

@RogerJL RogerJL deleted the ChartPolePR3 branch May 28, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pseudo-rnd-thoughts pseudo-rnd-thoughts pseudo-rnd-thoughts left review comments

@RedTachyon RedTachyon Awaiting requested review from RedTachyon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RogerJL @pseudo-rnd-thoughts