-
Notifications
You must be signed in to change notification settings - Fork 431
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
Add support for audio queries #579
Add support for audio queries #579
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think looks good. It would be good to do some testing with an actual algorithm.
For the python stage, it wouldn't be a "separate" return value, rather I think we should do a Dictionary of {"image": image_obs, "sound": sound_obs}
if sound_obs==True
.
We will need to update the observation_space
if we are doing that. I'm happy to make some of those changes if you are uncertain.
@jjshoots Do you have any thoughts?
7ab6368
to
92b5e3c
Compare
92b5e3c
to
4be5e22
Compare
I put together a simple algorithm which runs the image and sound data through separate feature encoders, concats, and then pushes through a simple conv network. Posting results for comparing sound_obs v no_sound_obs across a handful of atari games. Additionally, posting a video generated from the per-frame image+sound data for breakout. breakout.mp4
I'm not as familiar with the gym api, so would definitely appreciate help integrating the feature in the cleanest way. I've amended the commit to remove the draft portion of the changes to env.py. |
No worries, I have added the relevant code. def test_sound_obs():
env = gymnasium.make("ALE/MsPacman-v5", sound_obs=True)
with warnings.catch_warnings(record=True) as caught_warnings:
check_env(env.unwrapped, skip_render_check=True)
assert caught_warnings == [], [caught.message.args[0] for caught in caught_warnings] Another note is our pre-commit raised the following issue. Could you add this. check that executables have shebangs.....................................Failed
- hook id: check-executables-have-shebangs
- exit code: 1
src/ale/common/SoundRaw.cxx: marked executable but has no (or invalid) shebang!
If it isn't supposed to be executable, try: `chmod -x src/ale/common/SoundRaw.cxx`
If on Windows, you may also need to: `git add --chmod=-x src/ale/common/SoundRaw.cxx`
If it is supposed to be executable, double-check its shebang.
src/ale/common/SoundRaw.hxx: marked executable but has no (or invalid) shebang!
If it isn't supposed to be executable, try: `chmod -x src/ale/common/SoundRaw.hxx`
If on Windows, you may also need to: `git add --chmod=-x src/ale/common/SoundRaw.hxx`
If it is supposed to be executable, double-check its shebang. Otherwise, I suspect that we might be close to ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gkennickell Sorry, one last change, could you add some documentation to docs/cpp_interface.md
on accessing the audio queries.
Then we should be good to merge
Also could you run pip install pre-commit
and pre-commit run --all-files
in the project root
3989ca3
to
98fb00c
Compare
Added simple documentation in docs/cpp_interface.md + fixed pre-commit tests. Please let me know if more documentation is warranted (for instance, in environments.md) or minimal is a good start. |
98fb00c
to
7a28271
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I'll add more documentation later
Adds audio support queries based on #233 updated to work with latest. This is a simplified/refactored version, discussion in: #577.
The atari_env.py support in the initial commit is not final, stubbed in for feedback purposes.