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.

Sign up for GitHub

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

Jump to bottom

[Feature] Unify composite dist method signatures with other dists #981

Merged
merged 23 commits into from
Sep 12, 2024
Merged

[Feature] Unify composite dist method signatures with other dists #981

merged 23 commits into from
Sep 12, 2024

Conversation

albertbou92
Copy link
Contributor

Description

Describe your changes in detail.

Motivation and Context

Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.
You can use the syntax close #15213 if this solves the issue #15213

  • I have raised an issue to propose this change (required for new features and bug fixes)

Types of changes

What types of changes does your code introduce? Remove all that do not apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds core functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)
  • Example (update in the folder of examples)

Checklist

Add a method to compute the entropy for composite distributions

  • I have read the CONTRIBUTION guide (required)
  • My change requires a change to the documentation.
  • I have updated the tests accordingly (required for a bug fix or a new feature).
  • I have updated the documentation accordingly.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 2, 2024
@albertbou92 albertbou92 mentioned this pull request Sep 2, 2024
Merged
@vmoens
Copy link
Contributor

vmoens commented Sep 3, 2024

As mentioned here, for consistency we should have entropy and log_prob return tensors IMO

@albertbou92 albertbou92 changed the title Composite dist entropy [Feature] Unify composite dist method signatures with other dists Sep 7, 2024
@albertbou92 albertbou92 mentioned this pull request Sep 7, 2024
Closed
10 tasks
@vmoens vmoens added enhancement New feature or request Quality labels Sep 9, 2024
@albertbou92 albertbou92 requested a review from vmoens September 10, 2024 03:17
vmoens
vmoens reviewed Sep 10, 2024
View reviewed changes
tensordict/nn/distributions/composite.py Outdated Show resolved Hide resolved
tensordict/nn/distributions/composite.py Outdated Show resolved Hide resolved
tensordict/nn/distributions/composite.py Outdated Show resolved Hide resolved
tensordict/nn/distributions/composite.py Outdated Show resolved Hide resolved
tensordict/nn/distributions/composite.py Outdated Show resolved Hide resolved
tensordict/nn/distributions/composite.py Outdated Show resolved Hide resolved
tensordict/nn/distributions/composite.py Outdated Show resolved Hide resolved
albertbou92 and others added 4 commits September 10, 2024 06:10
@albertbou92 @vmoens
Update tensordict/nn/distributions/composite.py
b487a22 
Co-authored-by: Vincent Moens <vincentmoens@gmail.com>
@albertbou92 @vmoens
Update tensordict/nn/distributions/composite.py
f8e7bc6 
Co-authored-by: Vincent Moens <vincentmoens@gmail.com>
@albertbou92 @vmoens
Update tensordict/nn/distributions/composite.py
147c8a4 
Co-authored-by: Vincent Moens <vincentmoens@gmail.com>
@albertbou92
improve docstrings
92bc1f3
@albertbou92 albertbou92 requested a review from vmoens September 12, 2024 02:28
vmoens
vmoens reviewed Sep 12, 2024
View reviewed changes
Copy link
Contributor

@vmoens vmoens left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

tensordict/nn/distributions/composite.py Outdated Show resolved Hide resolved
@vmoens
Copy link
Contributor

vmoens commented Sep 12, 2024 •
edited
Loading

I had to make a few editions for BC-compatibility and also make sure that the warning was triggered only when required.

@vmoens
Copy link
Contributor

vmoens commented Sep 12, 2024

Merging, will fix torchrl's tests subsequently

@vmoens vmoens merged commit d4f8eee into pytorch:main Sep 12, 2024
37 of 45 checks passed
@vmoens vmoens mentioned this pull request Sep 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vmoens vmoens vmoens left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. enhancement New feature or request Quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@albertbou92 @vmoens @facebook-github-bot