这是indexloc提供的服务，不要输入任何密码
Skip to content

change Batch.empty to in-place fill; add copy option for Batch construction #110

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 14 commits into from
Jul 6, 2020
Merged

change Batch.empty to in-place fill; add copy option for Batch construction #110

merged 14 commits into from
Jul 6, 2020

Conversation

youkaichao
Copy link
Collaborator

  • I have marked all applicable categories:
    • exception-raising fix
    • algorithm implementation fix
    • documentation modification
    • new feature
  • If applicable, I have mentioned the relevant/related issue(s)

Less important but also useful:

  • I have visited the source website, and in particular read the known issues
  • I have searched through the issue tracker and issue categories for duplicates
  • I have mentioned version numbers, operating system and environment, where applicable: 
    import tianshou, torch, sys
print(tianshou.__version__, torch.__version__, sys.version, sys.platform)

@Trinkle23897 Trinkle23897 self-requested a review July 6, 2020 04:54
@duburcqa
Copy link
Collaborator

duburcqa commented Jul 6, 2020

Give me a few minutes to review your PR please.

Trinkle23897
Trinkle23897 reviewed Jul 6, 2020
View reviewed changes
Trinkle23897
Trinkle23897 reviewed Jul 6, 2020
View reviewed changes
duburcqa
duburcqa reviewed Jul 6, 2020
View reviewed changes
duburcqa
duburcqa reviewed Jul 6, 2020
View reviewed changes
duburcqa
duburcqa reviewed Jul 6, 2020
View reviewed changes
@codecov-commenter
Copy link

Codecov Report

Merging #110 into master will increase coverage by 0.00%.
The diff coverage is 94.11%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #110   +/-   ##
=======================================
  Coverage   84.41%   84.42%           
=======================================
  Files          29       29           
  Lines        1887     1875   -12     
=======================================
- Hits         1593     1583   -10     
+ Misses        294      292    -2
Flag Coverage Δ
#unittests 84.42% <94.11%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
tianshou/data/batch.py 87.63% <94.11%> (+0.53%) ⬆️
tianshou/policy/modelfree/sac.py 86.02% <0.00%> (-0.58%) ⬇️
tianshou/env/vecenv.py 71.22% <0.00%> (-0.14%) ⬇️
tianshou/policy/modelfree/dqn.py 97.36% <0.00%> (-0.07%) ⬇️
tianshou/policy/modelfree/ddpg.py 98.71% <0.00%> (-0.05%) ⬇️
tianshou/data/buffer.py 92.81% <0.00%> (-0.04%) ⬇️
tianshou/env/atari.py 0.00% <0.00%> (ø)
tianshou/policy/modelfree/td3.py 100.00% <0.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 db0e2e5...050f569. Read the comment docs.

@Trinkle23897
Copy link
Collaborator

@duburcqa I update the code and you can have a review.

duburcqa
duburcqa reviewed Jul 6, 2020
View reviewed changes
duburcqa
duburcqa reviewed Jul 6, 2020
View reviewed changes
@Trinkle23897
Copy link
Collaborator

@duburcqa any more comments?

@duburcqa
Copy link
Collaborator

duburcqa commented Jul 6, 2020

No it's fine ! Good job to both of you 👍

@Trinkle23897 Trinkle23897 merged commit 8913bf3 into thu-ml:master Jul 6, 2020
@youkaichao youkaichao deleted the batch_efficiency branch July 8, 2020 02:41
BFAnas pushed a commit to BFAnas/tianshou that referenced this pull request May 5, 2024
@youkaichao @Trinkle23897
change Batch.empty to in-place fill; add copy option for Batch constr…
29ce104 
…uction (thu-ml#110)

* in-place empty_ for Batch

* change Batch.empty to in-place fill; add copy option for Batch construction

* type signiture & remove shadow names for copy

* add doc for data type (only support numbers and object data type)

* add unit test for Batch copy

* fix pep8

* add test case for Batch.empty

* doc fix

* fix pep8

* use object to test Batch

* test commit

* refact

* change Batch(copy) testcase

* minor fix

Co-authored-by: Trinkle23897 <463003665@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duburcqa duburcqa duburcqa approved these changes

@Trinkle23897 Trinkle23897 Trinkle23897 approved these changes

Assignees

@youkaichao youkaichao

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@youkaichao @duburcqa @codecov-commenter @Trinkle23897