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

Add calibration to CQL as in CalQL paper arXiv:2303.05479 #915

Merged
merged 48 commits into from
Oct 3, 2023

Conversation

BFAnas
Copy link
Contributor

@BFAnas BFAnas commented Aug 17, 2023

  • I have marked all applicable categories:
    • exception-raising fix
    • algorithm implementation fix
    • documentation modification
    • new feature
  • I have reformatted the code using make format (required)
  • I have checked the code using make commit-checks (required)
  • If applicable, I have mentioned the relevant/related issue(s)
  • If applicable, I have listed every items in this Pull Request below

@MischaPanch
Copy link
Collaborator

If possible, pls wait until #912 is merged, as it will certainly introduce conflicts. It is now ready to be merged (CI is green), so I hope it will be quick

@BFAnas
Copy link
Contributor Author

BFAnas commented Aug 17, 2023

Yes sure no problem. I close this one for now until after your merge.

@BFAnas BFAnas closed this Aug 17, 2023
@MischaPanch
Copy link
Collaborator

Thank you, appreciate it!

@MischaPanch
Copy link
Collaborator

You can go for it now ;)

@BFAnas
Copy link
Contributor Author

BFAnas commented Aug 23, 2023

Awsome, congrats on the big Merge!

@BFAnas BFAnas reopened this Aug 23, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2023

Codecov Report

Merging #915 (6f4689e) into master (6449a43) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #915      +/-   ##
==========================================
+ Coverage   90.74%   90.75%   +0.01%     
==========================================
  Files          72       72              
  Lines        5292     5321      +29     
==========================================
+ Hits         4802     4829      +27     
- Misses        490      492       +2     
Flag Coverage Δ
unittests 90.75% <100.00%> (+0.01%) ⬆️

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

Files Coverage Δ
tianshou/data/batch.py 92.03% <ø> (ø)
tianshou/data/buffer/base.py 98.96% <100.00%> (+<0.01%) ⬆️
tianshou/data/buffer/her.py 90.52% <100.00%> (+0.20%) ⬆️
tianshou/data/collector.py 93.47% <100.00%> (ø)
tianshou/data/types.py 100.00% <100.00%> (ø)
tianshou/policy/base.py 80.33% <100.00%> (+0.33%) ⬆️
tianshou/policy/imitation/cql.py 100.00% <100.00%> (ø)
tianshou/policy/modelbased/psrl.py 97.56% <100.00%> (+0.03%) ⬆️
tianshou/trainer/base.py 97.76% <100.00%> (+0.02%) ⬆️

... and 2 files with indirect coverage changes

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

@MischaPanch
Copy link
Collaborator

@BFAnas I will try review this in the near future. Could you pls resolve the conflicts with master? We now have a new built and the newest formatting (for py3.11)

@BFAnas
Copy link
Contributor Author

BFAnas commented Sep 30, 2023

Hi @MischaPanch,
Sorry I was a little busy this week, I merged your changes now. I fixed some pep8 errors but now the tests are failing because of override "Module not found error", not sure if it's worth have it as a dependency. I think if we can do without it it'll be better.
Btw I don't find the "Allow anyone to contribute checkbox" in my fork settings.

@MischaPanch
Copy link
Collaborator

@BFAnas Thanks! The overrides dependency will be added in #950 anyway, so might as well add it here. Pls run poetry add overrides and commit the pyproject and poetry.lock file (after a round of poe format).

In python 3.12 override will become part of the language, so might as well start using it now. For tianshou it's even particularly useful. After that, the MR is ready for merge, thank you a lot for the contribution!

MischaPanch
MischaPanch previously approved these changes Sep 30, 2023
@BFAnas
Copy link
Contributor Author

BFAnas commented Oct 2, 2023

@BFAnas Thanks! The overrides dependency will be added in #950 anyway, so might as well add it here. Pls run poetry add overrides and commit the pyproject and poetry.lock file (after a round of poe format).

In python 3.12 override will become part of the language, so might as well start using it now. For tianshou it's even particularly useful. After that, the MR is ready for merge, thank you a lot for the contribution!

Done!

tianshou/policy/base.py Outdated Show resolved Hide resolved
MischaPanch
MischaPanch previously approved these changes Oct 2, 2023
@MischaPanch
Copy link
Collaborator

MischaPanch commented Oct 2, 2023

@Trinkle23897 this one is ready to merge, but the GPU runner seems to have problems, and I can't click the merge button. Could you do that, or fix the GPU runner? Alternatively, you could change the repository settings such that I can merge without all checks being green.

EDIT: almost ready to merge, see comments below

tianshou/policy/base.py Outdated Show resolved Hide resolved
tianshou/policy/base.py Outdated Show resolved Hide resolved
tianshou/policy/base.py Outdated Show resolved Hide resolved
test/policy/test_base.py Outdated Show resolved Hide resolved
tianshou/policy/imitation/cql.py Show resolved Hide resolved
@Trinkle23897 Trinkle23897 merged commit c30b4ab into thu-ml:master Oct 3, 2023
5 checks passed
noobsandnerds added a commit to noobsandnerds/tianshou that referenced this pull request Aug 7, 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.

4 participants