Feat/refactor collector #1063
Merged
Feat/refactor collector #1063
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MischaPanch
requested changes
Feb 23, 2024
|
Failing pipeline needs attention before the next review |
Still an off-by-one error somewhere
Previously the call to iter would reset the buffer, but some tests and scripts rely on collecting transitions prior to looping over the trainer
1. Call reset on collector before collecting 2. Collector logic has changed if one first collects some steps and then collects n_episodes. In the prior implementations, it would not reset the env, and thus not collect the desired number of full episodes, instead starting wherever it already was
…FER. IMPORTANT - this needs fixing!
MischaPanch
reviewed
Mar 26, 2024
MischaPanch
approved these changes
Mar 26, 2024
|
@Trinkle23897 this is ready for review now, @bordeauxred and I worked on this together, so it's reviewed from my side. There are only very minor breaking changes, see PR description Overall, I think it's a good step towards a more readable and transparent Collector. We have encountered many issues while working on that, so there are many new TODOs. We'll address them later on, and I'll add a new notebook documenting "gotchas" in Batch and Buffer in a separate PR. I think a quick review from your side should be enough, understanding all details of the changes would be time consuming. Long story short: collector implementations rely less on state, are better typed and are more readable now. Hacky batch things have been marked as such explicitly |
8 tasks
|
I'll merge this now since it should be uncontroversial and it is blocking further development like #1077 . @Trinkle23897 if you have any comments, we can address them post-merge |
|
Thanks for doing that! |
ZhengLi1314
pushed a commit
to ZhengLi1314/tianshou_0.5.1
that referenced
this pull request
Apr 15, 2024
Closes: thu-ml#1058 ### Api Extensions - Batch received two new methods: `to_dict` and `to_list_of_dicts`. thu-ml#1063 - `Collector`s can now be closed, and their reset is more granular. thu-ml#1063 - Trainers can control whether collectors should be reset prior to training. thu-ml#1063 - Convenience constructor for `CollectStats` called `with_autogenerated_stats`. thu-ml#1063 ### Internal Improvements - `Collector`s rely less on state, the few stateful things are stored explicitly instead of through a `.data` attribute. thu-ml#1063 - Introduced a first iteration of a naming convention for vars in `Collector`s. thu-ml#1063 - Generally improved readability of Collector code and associated tests (still quite some way to go). thu-ml#1063 - Improved typing for `exploration_noise` and within Collector. thu-ml#1063 ### Breaking Changes - Removed `.data` attribute from `Collector` and its child classes. thu-ml#1063 - Collectors no longer reset the environment on initialization. Instead, the user might have to call `reset` expicitly or pass `reset_before_collect=True` . thu-ml#1063 - VectorEnvs now return an array of info-dicts on reset instead of a list. thu-ml#1063 - Fixed `iter(Batch(...)` which now behaves the same way as `Batch(...).__iter__()`. Can be considered a bugfix. thu-ml#1063 --------- Co-authored-by: Michael Panchenko <m.panchenko@appliedai.de>
ZhengLi1314
pushed a commit
to ZhengLi1314/tianshou_0.5.1
that referenced
this pull request
Apr 15, 2024
Closes: thu-ml#1058 ### Api Extensions - Batch received two new methods: `to_dict` and `to_list_of_dicts`. thu-ml#1063 - `Collector`s can now be closed, and their reset is more granular. thu-ml#1063 - Trainers can control whether collectors should be reset prior to training. thu-ml#1063 - Convenience constructor for `CollectStats` called `with_autogenerated_stats`. thu-ml#1063 ### Internal Improvements - `Collector`s rely less on state, the few stateful things are stored explicitly instead of through a `.data` attribute. thu-ml#1063 - Introduced a first iteration of a naming convention for vars in `Collector`s. thu-ml#1063 - Generally improved readability of Collector code and associated tests (still quite some way to go). thu-ml#1063 - Improved typing for `exploration_noise` and within Collector. thu-ml#1063 ### Breaking Changes - Removed `.data` attribute from `Collector` and its child classes. thu-ml#1063 - Collectors no longer reset the environment on initialization. Instead, the user might have to call `reset` expicitly or pass `reset_before_collect=True` . thu-ml#1063 - VectorEnvs now return an array of info-dicts on reset instead of a list. thu-ml#1063 - Fixed `iter(Batch(...)` which now behaves the same way as `Batch(...).__iter__()`. Can be considered a bugfix. thu-ml#1063 --------- Co-authored-by: Michael Panchenko <m.panchenko@appliedai.de>
ZhengLi1314
pushed a commit
to ZhengLi1314/tianshou_0.5.1
that referenced
this pull request
Apr 15, 2024
Closes: thu-ml#1058 ### Api Extensions - Batch received two new methods: `to_dict` and `to_list_of_dicts`. thu-ml#1063 - `Collector`s can now be closed, and their reset is more granular. thu-ml#1063 - Trainers can control whether collectors should be reset prior to training. thu-ml#1063 - Convenience constructor for `CollectStats` called `with_autogenerated_stats`. thu-ml#1063 ### Internal Improvements - `Collector`s rely less on state, the few stateful things are stored explicitly instead of through a `.data` attribute. thu-ml#1063 - Introduced a first iteration of a naming convention for vars in `Collector`s. thu-ml#1063 - Generally improved readability of Collector code and associated tests (still quite some way to go). thu-ml#1063 - Improved typing for `exploration_noise` and within Collector. thu-ml#1063 ### Breaking Changes - Removed `.data` attribute from `Collector` and its child classes. thu-ml#1063 - Collectors no longer reset the environment on initialization. Instead, the user might have to call `reset` expicitly or pass `reset_before_collect=True` . thu-ml#1063 - VectorEnvs now return an array of info-dicts on reset instead of a list. thu-ml#1063 - Fixed `iter(Batch(...)` which now behaves the same way as `Batch(...).__iter__()`. Can be considered a bugfix. thu-ml#1063 --------- Co-authored-by: Michael Panchenko <m.panchenko@appliedai.de>
ZhengLi1314
pushed a commit
to ZhengLi1314/tianshou_0.5.1
that referenced
this pull request
Apr 15, 2024
Closes: thu-ml#1058 ### Api Extensions - Batch received two new methods: `to_dict` and `to_list_of_dicts`. thu-ml#1063 - `Collector`s can now be closed, and their reset is more granular. thu-ml#1063 - Trainers can control whether collectors should be reset prior to training. thu-ml#1063 - Convenience constructor for `CollectStats` called `with_autogenerated_stats`. thu-ml#1063 ### Internal Improvements - `Collector`s rely less on state, the few stateful things are stored explicitly instead of through a `.data` attribute. thu-ml#1063 - Introduced a first iteration of a naming convention for vars in `Collector`s. thu-ml#1063 - Generally improved readability of Collector code and associated tests (still quite some way to go). thu-ml#1063 - Improved typing for `exploration_noise` and within Collector. thu-ml#1063 ### Breaking Changes - Removed `.data` attribute from `Collector` and its child classes. thu-ml#1063 - Collectors no longer reset the environment on initialization. Instead, the user might have to call `reset` expicitly or pass `reset_before_collect=True` . thu-ml#1063 - VectorEnvs now return an array of info-dicts on reset instead of a list. thu-ml#1063 - Fixed `iter(Batch(...)` which now behaves the same way as `Batch(...).__iter__()`. Can be considered a bugfix. thu-ml#1063 --------- Co-authored-by: Michael Panchenko <m.panchenko@appliedai.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #1058
Description (copied from newly established Changelog):
Api Extensions
to_dictandto_list_of_dicts. Feat/refactor collector #1063Collectors can now be closed, and their reset is more granular. Feat/refactor collector #1063CollectStatscalledwith_autogenerated_stats. Feat/refactor collector #1063Internal Improvements
Collectors rely less on state, the few stateful things are stored explicitly instead of through a.dataattribute. Feat/refactor collector #1063Collectors. Feat/refactor collector #1063exploration_noiseand within Collector. Feat/refactor collector #1063Breaking Changes
.dataattribute fromCollectorand its child classes. Feat/refactor collector #1063resetexpicitly or pass
reset_before_collect=True. Feat/refactor collector #1063iter(Batch(...)which now behaves the same way asBatch(...).__iter__(). Can be considered a bugfix. Feat/refactor collector #1063