Skip to content

[BugFix] Support list-based boolean masks for TensorDict #299

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
Merged
merged 16 commits into from
Jul 21, 2022
Merged

[BugFix] Support list-based boolean masks for TensorDict #299

merged 16 commits into from
Jul 21, 2022

Conversation

benoitdescamps
Copy link
Contributor

@benoitdescamps benoitdescamps commented Jul 19, 2022 •
edited
Loading

Description

Fixes issue #258

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

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

  • 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 Jul 19, 2022
@vmoens vmoens added the bug Something isn't working label Jul 19, 2022
@vmoens vmoens linked an issue Jul 19, 2022 that may be closed by this pull request
[Bug] Support list-based boolean masks for TensorDict #258
Closed
@vmoens
Copy link
Collaborator

vmoens commented Jul 20, 2022

Thanks for picking this up @benoitdescamps
I'm looking into the failing test.
There is a bunch of stuff that needs to be fixed for this PR to be mergeable, I'm taking care of it. Bear with me, I'll do a PR on your branch as soon as I'm done with it!

@vmoens
Copy link
Collaborator

vmoens commented Jul 20, 2022

Thanks for picking this up @benoitdescamps I'm looking into the failing test. There is a bunch of stuff that needs to be fixed for this PR to be mergeable, I'm taking care of it. Bear with me, I'll do a PR on your branch as soon as I'm done with it!

you can check the PR here benoitdescamps#1

@benoitdescamps benoitdescamps changed the title T126316008 TorchRL: Support list-based boolean masks for TensorDict Jul 20, 2022
@vmoens vmoens changed the title TorchRL: Support list-based boolean masks for TensorDict [BugFix] Support list-based boolean masks for TensorDict Jul 20, 2022
benoitdescamps and others added 3 commits July 20, 2022 15:29
@benoitdescamps
Merge pull request #1 from vmoens/bf_T126316008
054eca9 
BugFix: test setting values using mask, fix various bugs in masking tensordicts
@vmoens
Merge branch 'main' into bf_T126316008
e753a01
@benoitdescamps
Merge pull request #2 from vmoens/bf_T126316008
fa80503 
Fix CI
@vmoens
Copy link
Collaborator

vmoens commented Jul 21, 2022

We have device issues in the tests, do you think you can handle it @benoitdescamps ?

@benoitdescamps
Copy link
Contributor Author

We have device issues in the tests, do you think you can handle it @benoitdescamps ?

yes, I will have a go at it

vmoens
vmoens reviewed Jul 21, 2022
View reviewed changes
test/test_tensordict.py Outdated
"key2": torch.randn(4, 5, 10, device=device),
}
mask = torch.zeros(4, 5, dtype=torch.bool, device=device).bernoulli_()
mask_list = mask.numpy().tolist()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be moved to cpu too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@benoitdescamps benoitdescamps force-pushed the T126316008 branch 2 times, most recently from 395cb7c to 1c27607 Compare July 21, 2022 14:04
@benoitdescamps benoitdescamps requested a review from vmoens July 21, 2022 15:23
vmoens
vmoens approved these changes Jul 21, 2022
View reviewed changes
Copy link
Collaborator

@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 for this feature! Also helped to uncover a couple of bugs, which is great!

@vmoens vmoens merged commit 5c74ab7 into pytorch:main Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vmoens vmoens vmoens approved these changes

Assignees
No one assigned
Labels
bug Something isn't working CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Support list-based boolean masks for TensorDict
3 participants
@benoitdescamps @vmoens @facebook-github-bot