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

Fix reading shared memory for Tuple and Dict spaces #941

Conversation

pseudo-rnd-thoughts
Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts commented Feb 27, 2024

Description

Soooo shared memory with Tuple or Dict spaces has been broken since the original implementation (5 years old).
We didn't catch this as the expected tests should be assert data_equivalence(read_samples, samples) and were actually just data_equivalence(...). (yes I have checked and can't find anymore instances of this bug in testing)

As a result, the returned samples were roundtripped are

space = Tuple((Discrete(3), Discrete(4)))

num = 2
samples = (space.sample() for _ in range(num))  # ((2, 0), (2, 3))
shared_memory = create_shared_memory(space, n=num, ctx=mp)

for i, sample in enumerate(samples):
    write_to_shared_memory(space, i, sample, shared_memory)

read_samples = read_from_shared_memory(space, shared_memory, n=num)  # ((2, 2), (0, 3))

This PR fixes the bug

@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit 36598b9 into Farama-Foundation:main Feb 27, 2024
11 checks passed
pseudo-rnd-thoughts added a commit to pseudo-rnd-thoughts/Gymnasium that referenced this pull request Sep 19, 2024
@pseudo-rnd-thoughts pseudo-rnd-thoughts mentioned this pull request Sep 19, 2024
@pseudo-rnd-thoughts
Copy link
Member Author

Ironically, the original implementation was correct
The problem was the test was incorrect for these particular cases, we should have been using iterate(read_samples) rather than just iterate like normal.
This PR is reverted in #1165

pseudo-rnd-thoughts added a commit that referenced this pull request Sep 19, 2024
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.

1 participant