Skip to content

Conversation

shwang
Copy link
Member

@shwang shwang commented Mar 12, 2021

Fixes an ~"negative probability out-of-bounds" error I ran into when I was training.

See: https://pytorch.org/docs/stable/distributions.html#bernoulli for func signature

Led to an error when I was training.
@shwang shwang requested a review from AdamGleave March 12, 2021 23:22
@codecov
Copy link

codecov bot commented Mar 12, 2021

Codecov Report

Merging #273 (bcae317) into master (9ddd6fa) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #273   +/-   ##
=======================================
  Coverage   90.01%   90.01%           
=======================================
  Files          74       74           
  Lines        5146     5146           
=======================================
  Hits         4632     4632           
  Misses        514      514           
Impacted Files Coverage Δ
src/imitation/rewards/common.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ddd6fa...bcae317. Read the comment docs.

Copy link
Member

@AdamGleave AdamGleave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

This seems like a badly designed function: Bernoulli(probs, logits) is illegal (can only specify one), you have to specify logits via keyword argument. Would have been better if it had been Bernoulli(*, probs, logits) to force people to always use keyword arguments. Oh well.

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