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

[Tiny] Fix scrabble test #289

Merged
merged 3 commits into from
Feb 20, 2024
Merged

[Tiny] Fix scrabble test #289

merged 3 commits into from
Feb 20, 2024

Conversation

alexhernandezgarcia
Copy link
Owner

One of the Scrabble tests was disabled in a previous PR because it was failing. This PR fixes it. The reason for the fail was that the state had to be copied because the method step() of the Scrabble env does not copy the state unlike other envs.

@josephdviviano
Copy link
Collaborator

LGTM - can you add a comment explaining why copy is required here?

Copy link
Collaborator

@josephdviviano josephdviviano left a comment

Choose a reason for hiding this comment

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

looks great but would appreciate a comment as to why copy is needed for our future selves.

@alexhernandezgarcia
Copy link
Owner Author

@josephdviviano copy is needed because of the following:

  • The state of the Scrabble is a list of ints.
  • This particular test stores a list of states in a trajectory as it samples actions. So a list of lists, called states_trajectory_fw in the code.
  • Because the step() method of the environment (which applies the actions) does not copy the state, if the test does not copy the state either, then the states stored in states_trajectory_fw would be the same object. That is, they would be updated as actions are made and we will not have all the states in the trajectory but just the last state repeated multiple times.
  • This was not a problem in other environments because step() does copy the state, so we're good.

Does that make sense?

@josephdviviano
Copy link
Collaborator

@alexhernandezgarcia it does make sense indeed - I just meant a comment in the test code so people in the future can also understand why there need be a copy there.

@alexhernandezgarcia
Copy link
Owner Author

@alexhernandezgarcia it does make sense indeed - I just meant a comment in the test code so people in the future can also understand why there need be a copy there.

Oh, but there are multiple places where copy is used and the explanation is kind of long :/ Should I add a general comment at the beginning of the script?

@alexhernandezgarcia
Copy link
Owner Author

Comment about copies added here.

@josephdviviano
Copy link
Collaborator

I added some inline comments as well. Thanks!

@josephdviviano josephdviviano merged commit 05e6867 into main Feb 20, 2024
1 check passed
@alexhernandezgarcia alexhernandezgarcia deleted the fix-reversible-test-scrabble branch February 20, 2024 16:00
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.

2 participants