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

Env creation tutorial 2 : grid size inconsistent #1036

Merged
merged 4 commits into from Jul 25, 2023

Conversation

BertrandDecoster
Copy link
Contributor

@BertrandDecoster BertrandDecoster commented Jul 21, 2023

There were two mistakes

  • The grid is 7x7 but the guard starts at position (7,7) which is out of bounds
  • The type of the grid used in render() was integers and it conflicts with the characters we assign in the function

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue), Depends on # (pull request)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • 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 run pytest -v and no errors are present.
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I solved any possible warnings that pytest -v has generated that are related to my code to the best of my knowledge.
  • 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

Most of those at not relevant, it's a doc change

There were two mistakes
 - The grid is 8x8, not 7x7 : the guard starts at position (7,7)
 - The type was integers and it conflicts with the characters we assign in the function
@BertrandDecoster
Copy link
Contributor Author

For info, I want to have this PR validated before doing a C/C for tutorial 3 of env creation

@BertrandDecoster BertrandDecoster changed the title Env creation tutorial 2 : render function was broken Env creation tutorial 2 : grid size inconsistent Jul 21, 2023
@@ -104,7 +104,7 @@ def step(self, actions):
return observations, rewards, terminations, truncations, infos

def render(self):
grid = np.zeros((7, 7))
grid = np.full((7,7), ' ', dtype='U1')
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does the second argument to np.full do here? And I googled around to see what exactly dtype U1 is but I'm confused about how this site says https://note.nkmk.me/en/python-numpy-dtype-astype/ the dtype should be <U1?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The render space doesn't have to be the same as the observation space to be fair, but I can see how if we're overriding an array of integers with characters as the input that's a little sloppy. Granted, numpy lets you do that so it's like, ultimately it doesn't matter and is arguably cleaner code, but not as strictly correct. Could do it as a list of lists of size 49 or some other data type as well, but this is probably cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure it doesn't matter? It doesn't seem to want to write down a char in a array of floats

Screenshot 2023-07-22 at 09 12 35

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But you're right that numpy does type inference and selects an appropriate type, so there's no need to manually specify that it's Unicode, I can remove U1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see, thanks for testing it out. Looks good just doing full with a space character

@elliottower
Copy link
Collaborator

If you can fix pre commit I’d be happy to merge

@elliottower elliottower merged commit 83d243c into Farama-Foundation:master Jul 25, 2023
33 checks passed
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.

None yet

2 participants