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

Using dist.mode instead of logits.argmax. More compact. #1066

Merged
merged 5 commits into from
Mar 2, 2024

Conversation

arnaujc91
Copy link
Contributor

@arnaujc91 arnaujc91 commented Feb 29, 2024

changed all the occurrences where picking an action deterministically

  • from: using the outputs of the actor network.
  • to: using the mode of the PyTorch distribution.

This was agreed with @MischaPanch.

Please make sure everything is alright.

@MischaPanch
Copy link
Collaborator

@arnaujc91 a test fails due to cryptic float/rounding errors. Could you introduce a rounding to say 5 digits to make sure the test succeeds?

@arnaujc91
Copy link
Contributor Author

@arnaujc91 a test fails due to cryptic float/rounding errors. Could you introduce a rounding to say 5 digits to make sure the test succeeds?

Please check test modifications.

@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.17%. Comparing base (7c970df) to head (a9a664d).

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1066      +/-   ##
==========================================
+ Coverage   88.16%   88.17%   +0.01%     
==========================================
  Files         100      100              
  Lines        8176     8172       -4     
==========================================
- Hits         7208     7206       -2     
+ Misses        968      966       -2     
Flag Coverage Δ
unittests 88.17% <100.00%> (+0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MischaPanch
Copy link
Collaborator

@arnaujc91 pls resolve the conflicts with master

@arnaujc91
Copy link
Contributor Author

@arnaujc91 pls resolve the conflicts with master

done

@MischaPanch MischaPanch merged commit 1aee41f into thu-ml:master Mar 2, 2024
4 of 5 checks passed
ZhengLi1314 pushed a commit to ZhengLi1314/tianshou_0.5.1 that referenced this pull request Apr 15, 2024
changed all the occurrences where an action is selected deterministically

- **from**: using the outputs of the actor network.
- **to**: using the mode of the PyTorch distribution.

---------

Co-authored-by: Arnau Jimenez <[email protected]>
ZhengLi1314 pushed a commit to ZhengLi1314/tianshou_0.5.1 that referenced this pull request Apr 15, 2024
changed all the occurrences where an action is selected deterministically

- **from**: using the outputs of the actor network.
- **to**: using the mode of the PyTorch distribution.

---------

Co-authored-by: Arnau Jimenez <[email protected]>
ZhengLi1314 pushed a commit to ZhengLi1314/tianshou_0.5.1 that referenced this pull request Apr 15, 2024
changed all the occurrences where an action is selected deterministically

- **from**: using the outputs of the actor network.
- **to**: using the mode of the PyTorch distribution.

---------

Co-authored-by: Arnau Jimenez <[email protected]>
ZhengLi1314 pushed a commit to ZhengLi1314/tianshou_0.5.1 that referenced this pull request Apr 15, 2024
changed all the occurrences where an action is selected deterministically

- **from**: using the outputs of the actor network.
- **to**: using the mode of the PyTorch distribution.

---------

Co-authored-by: Arnau Jimenez <[email protected]>
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.

3 participants