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

Buffer refactoring to support batch over batch reliably #93

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.

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

Merged
merged 17 commits into from
Jun 25, 2020

Conversation

duburcqa
Copy link
Collaborator

@duburcqa duburcqa commented Jun 24, 2020

  • Add axis option to Batch stack method
  • Add support of item assignment for Batch (partial item assignment also supported)
  • Generalized slicing using any type that is usually accepted by np.ndarray for both Batch and Buffer classes
  • Improve robustness of Batch implement by avoiding using direct assignment
  • Remove metadata attribute of Buffer class and inherit directly from Batch. It greatly simplify and clarify the implementation.

This PR incidently fixes the support of deeply nested Batch over Batch for Buffers.

@duburcqa duburcqa force-pushed the buffer_batch_over_batch branch 2 times, most recently from aaac27a to 152b69c Compare June 24, 2020 13:47
@duburcqa duburcqa changed the title Fix support of batch over batch for Buffer WIP: Fix support of batch over batch for Buffer Jun 24, 2020
@duburcqa duburcqa force-pushed the buffer_batch_over_batch branch from 152b69c to 076619a Compare June 24, 2020 13:50
@Trinkle23897
Copy link
Collaborator

Do you think the _meta should be removed as Batch?

@duburcqa
Copy link
Collaborator Author

duburcqa commented Jun 24, 2020

Since you are asking, yes I think so. It makes everything pretty difficult to understand and maintain without adding any feature at the end. However, I'm afraid I don't really understand how this mechanism is working right now, so I'm not able to refactor this submodule by myself. It would be awesome to take advantage of the newly added flexibility of Batch to simplify the implement of this module.

@Trinkle23897
Copy link
Collaborator

It is almost the same as the previous Batch implementation. The difference includes automatically create the np.array with given data. Nothing special.

@duburcqa
Copy link
Collaborator Author

Ok I'll have a look at it.

@duburcqa duburcqa force-pushed the buffer_batch_over_batch branch from 076619a to 72f01e5 Compare June 24, 2020 14:39
@duburcqa
Copy link
Collaborator Author

I think I should be able to handle it finally. I let you know.

@duburcqa duburcqa force-pushed the buffer_batch_over_batch branch 20 times, most recently from b66a836 to e4888f8 Compare June 24, 2020 17:28
@duburcqa duburcqa force-pushed the buffer_batch_over_batch branch from ffc372f to 184e26a Compare June 25, 2020 09:34
@duburcqa
Copy link
Collaborator Author

Doing batch.obs[indices] = ... (item assignment) triggers this broadcasting, while batch.obs = ... (direct assignment) does not. It is important, since item assignment must not break item access, which means doing afterward batch.obs[indices]must be valid under any condition, which is not the case if you do not broadcast, while direct assignment has not such guarantee.

@duburcqa duburcqa force-pushed the buffer_batch_over_batch branch 3 times, most recently from 54d8f2c to 59ee0e7 Compare June 25, 2020 10:02
@duburcqa duburcqa force-pushed the buffer_batch_over_batch branch from 59ee0e7 to 551fac2 Compare June 25, 2020 10:07
@duburcqa
Copy link
Collaborator Author

duburcqa commented Jun 25, 2020

Another solution is that this new feature can be only appeared in Buffer instead of Batch.

Finally I will do this, since it is impossible to get a reliable behaviour using broadcasting (lower level nested Batch instances have no idea of the size of the top Batch, making it impossible to perform broadcast), and it was pretty hard to maintain.

@duburcqa duburcqa force-pushed the buffer_batch_over_batch branch 7 times, most recently from 51b3027 to 6fea54d Compare June 25, 2020 12:06
@duburcqa duburcqa force-pushed the buffer_batch_over_batch branch from 6fea54d to 3c4085a Compare June 25, 2020 12:09
@duburcqa duburcqa changed the title Fix support of batch over batch for Buffer Buffer refactoring to support of batch over batch reliably Jun 25, 2020
@duburcqa duburcqa changed the title Buffer refactoring to support of batch over batch reliably Buffer refactoring to support batch over batch reliably Jun 25, 2020
@duburcqa
Copy link
Collaborator Author

@Trinkle23897 PR ready to be merged ! All tests passing successfully finally !

@Trinkle23897
Copy link
Collaborator

By the way, I found that in #92, the overhead of Batch.__getitem__ grows 20% because of calling _valid_bounds. Do you have any idea?

@Trinkle23897 Trinkle23897 merged commit 3086b5c into thu-ml:master Jun 25, 2020
@duburcqa
Copy link
Collaborator Author

YEs, I'm on it !

@duburcqa
Copy link
Collaborator Author

@Trinkle23897 Can you check this branch ? #95

@duburcqa duburcqa deleted the buffer_batch_over_batch branch June 25, 2020 12:46
@Trinkle23897 Trinkle23897 linked an issue Jun 29, 2020 that may be closed by this pull request
BFAnas pushed a commit to BFAnas/tianshou that referenced this pull request May 5, 2024
* Fix support of batch over batch for Buffer.

* Do not use internal __dict__ attribute to store batch data since it breaks inheritance.

* Various fixes.

* Improve robustness of Batch/Buffer by avoiding direct attribute assignment. Buffer refactoring.

* Add axis optional argument to Batch stack method.

* Add item assignment to Batch class.

* Fix list support for Buffer.

* Convert list to np.array by default for efficiency.

* Add missing unit test for Batch. Fix unit tests.

* Batch item assignment is now robust to key order.

* Do not use getattr/setattr explicity for simplicity.

* More flexible __setitem__.

* Fixes

* Remove broacasting at Batch level since it is unreliable.

* Forbid item assignement for inconsistent batches.

* Implement broadcasting at Buffer level.

* Add more unit test for Batch item assignment.

Co-authored-by: Alexis Duburcq <alexis.duburcq@wandercraft.eu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch to Numpy compatibility
3 participants