Skip to content

Conversation

@ervteng
Copy link
Contributor

@ervteng ervteng commented Feb 27, 2020

Proposed change(s)

Rather than have the output distribution built into the Policy, we separate these out into a separate class. Child classes of OutputDistribution include both continuous and discrete versions. This opens the door to adding more types of output distributions in the future without major changes to Policy code.

Types of change(s)

  • Bug fix
  • New feature
  • Code refactor
  • Breaking change
  • Documentation update
  • Other (please describe)

Checklist

  • Added tests that prove my fix is effective or that my feature works
  • Updated the changelog (if applicable)
  • Updated the documentation (if applicable)
  • Updated the migration guide (if applicable)

Other comments

@ervteng ervteng requested a review from andrewcoh February 27, 2020 01:11
pass

@abc.abstractproperty
def total_log_probs(self) -> tf.Tensor:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this equivalent to reduce_sum(log_probs, axis=1) or something different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty much - are we 100% sure that reduce_sum would work for all distribution types?

)
# Make entropy the right shape
self.entropy = tf.ones_like(tf.reshape(mu[:, 0], [-1])) * single_dim_entropy
self.entropy = distribution.entropy
Copy link
Contributor

Choose a reason for hiding this comment

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

can we aggregate all occurrences of this line i.e. line 283

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 will do it in a follow-up PR. TODO: broader discussion about standardization of discrete and continuous (requires inference changes on C#)

@ervteng ervteng marked this pull request as ready for review February 27, 2020 23:25
@ervteng ervteng merged commit 78a218b into master Feb 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the develop-distributions branch February 28, 2020 21:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants