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

[BugFix] Fix SyncDataCollector reset #571

Merged
merged 2 commits into from
Oct 15, 2022
Merged

[BugFix] Fix SyncDataCollector reset #571

merged 2 commits into from
Oct 15, 2022

Conversation

jrobine
Copy link
Contributor

@jrobine jrobine commented Oct 14, 2022 •
edited
Loading

Description

Fix #570
close #570

  • I have raised an issue to propose this change

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • 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.

@jrobine
Update collectors.py
7bab058 
Fix SyncDataCollector reset
@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 Oct 14, 2022
@codecov
Copy link

codecov bot commented Oct 14, 2022 •
edited
Loading

Codecov Report

Merging #571 (7bab058) into main (872dd2d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 7bab058 differs from pull request most recent head 64bb6d9. Consider uploading reports for the commit 64bb6d9 to get more accurate results

@@           Coverage Diff           @@
##             main     #571   +/-   ##
=======================================
  Coverage   86.90%   86.90%           
=======================================
  Files         121      121           
  Lines       21802    21802           
=======================================
+ Hits        18946    18947    +1     
+ Misses       2856     2855    -1
Flag Coverage Δ
linux-cpu 85.23% <100.00%> (+<0.01%) ⬆️
linux-gpu 86.68% <100.00%> (+<0.01%) ⬆️
linux-outdeps-gpu 75.45% <100.00%> (+0.01%) ⬆️
linux-stable-cpu 85.21% <100.00%> (+<0.01%) ⬆️
linux-stable-gpu 86.68% <100.00%> (+<0.01%) ⬆️
macos-cpu 84.99% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
torchrl/collectors/collectors.py 69.62% <100.00%> (ø)
torchrl/data/tensordict/tensordict.py 82.55% <0.00%> (+0.04%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

vmoens
vmoens reviewed Oct 15, 2022
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.

Thanks! Let's try without the update?

torchrl/collectors/collectors.py Outdated
@@ -585,7 +585,7 @@ def _reset_if_necessary(self) -> None:
else:
self._tensordict.zero_()

self._tensordict.update(self.env.reset(), inplace=True)
self._tensordict.update(self.env.reset(self._tensordict), inplace=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need the call to update.
env.reset() will update the tensordict.

@vmoens vmoens added the bug Something isn't working label Oct 15, 2022
@vmoens
Copy link
Contributor

vmoens commented Oct 15, 2022

I implemented some tests, let me know if they make sense

@vmoens vmoens merged commit 77beeb9 into pytorch:main Oct 15, 2022
@jrobine
Copy link
Contributor Author

jrobine commented Oct 15, 2022

Thanks! It seems like the step counts are also incorrect, so I think only assertions 313 and 322 are really necessary.

@vmoens
Copy link
Contributor

vmoens commented Oct 15, 2022

Do you mean "were incorrect" or are they still incorrect after this PR?

@jrobine
Copy link
Contributor Author

jrobine commented Oct 15, 2022

Sorry, they were incorrect

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
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] SyncDataCollector resets all workers instead of one
3 participants
@jrobine @vmoens @facebook-github-bot