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

Updated atari wrappers, fixed pre-commit #781

Merged
merged 4 commits into from
Dec 4, 2022

Conversation

Markus28
Copy link
Collaborator

@Markus28 Markus28 commented Dec 2, 2022

This PR addresses #772 (updates Atari wrappers to work with new Gym API) and some additional issues:

  • Pre-commit was using gitlab for flake8, which as of recently requires authentication -> Replaced with GitHub
  • Yapf was quietly failing in pre-commit. Changed it such that it fixes formatting in-place
  • There is an incompatibility between flake8 and yapf where yapf puts binary operators after the line break and flake8 wants it before the break. I added an exception for flake8.
  • Also require packaging in setup.py

My changes shouldn't change the behaviour of the wrappers for older versions, but please double check.
Idk whether it's just me, but there are always some incompatibilities between yapf and flake8 that need to resolved manually. It might make sense to try black instead.

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2022

Codecov Report

Merging #781 (e3f1a8f) into master (662af52) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #781      +/-   ##
==========================================
- Coverage   91.07%   91.05%   -0.02%     
==========================================
  Files          71       71              
  Lines        5077     5077              
==========================================
- Hits         4624     4623       -1     
- Misses        453      454       +1     
Flag Coverage Δ
unittests 91.05% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tianshou/data/buffer/her.py 90.58% <0.00%> (-1.18%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nuance1979
Copy link
Collaborator

Idk whether it's just me, but there are always some incompatibilities between yapf and flake8 that need to resolved manually. It might make sense to try black instead.

I concur. black seems to be more consistent than yapf.

@Trinkle23897 Trinkle23897 linked an issue Dec 4, 2022 that may be closed by this pull request
8 tasks
@Trinkle23897
Copy link
Collaborator

I agree we should change to black at some point.

@Trinkle23897 Trinkle23897 merged commit 4c3791a into thu-ml:master Dec 4, 2022
BFAnas pushed a commit to BFAnas/tianshou that referenced this pull request May 5, 2024
This PR addresses thu-ml#772 (updates Atari wrappers to work with new Gym API)
and some additional issues:

- Pre-commit was using gitlab for flake8, which as of recently requires
authentication -> Replaced with GitHub
- Yapf was quietly failing in pre-commit. Changed it such that it fixes
formatting in-place
- There is an incompatibility between flake8 and yapf where yapf puts
binary operators after the line break and flake8 wants it before the
break. I added an exception for flake8.
- Also require `packaging` in setup.py

My changes shouldn't change the behaviour of the wrappers for older
versions, but please double check.
Idk whether it's just me, but there are always some incompatibilities
between yapf and flake8 that need to resolved manually. It might make
sense to try black instead.
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.

about the gym version
4 participants