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 bug in masked, multi-dimensional state space multi-threaded envs #900

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Graeme22
Copy link

@Graeme22 Graeme22 commented Jun 8, 2023

This fixes what appears to be a copy/paste bug that affects multi-threaded envs where the state space of the wrapped environment has different dimensionality than the action space of that environment.

For an example environment that this breaks, see https://github.com/Graeme22/rlskedge

PR Checklist

  • Update NEWS.md?
  • Unit tests for all structs / functions?
  • Integration and correctness tests using a simple env?
  • PR Review?
  • Add or update documentation?
  • Write docstrings for new methods?

@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #900 (32604f2) into main (ea00fdf) will decrease coverage by 24.44%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #900       +/-   ##
==========================================
- Coverage   24.47%   0.04%   -24.44%     
==========================================
  Files         219     207       -12     
  Lines        7686    7361      -325     
==========================================
- Hits         1881       3     -1878     
- Misses       5805    7358     +1553     
Impacted Files Coverage Δ
...src/algorithms/policy_gradient/multi_thread_env.jl 0.00% <0.00%> (ø)

... and 64 files with indirect coverage changes

@jeremiahpslewis
Copy link
Member

@Graeme22 This looks like a good catch, thanks! Would you be able to contribute a test that would have caught the issue?

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