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.
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
Train High-Level Policies in Hierarchical Approaches #1053
Train High-Level Policies in Hierarchical Approaches #1053
Changes from 6 commits
a787e34
f6d1f11
daa84db
7ae2c6b
bb2e4da
628063d
d179ecf
515b3cf
8eddcd1
fa94bfc
11659b4
633ebf3
1c82390
2e973f5
3a805ea
7d92072
4696551
c11601f
90155e1
755acb4
8d07335
4754fd7
587672e
f16bc14
22cb83f
df8b7a6
422c036
5643b0e
ebe877e
e1a8727
2673f4f
03faec7
dd01d50
b20fcb1
7982f40
902bfa1
74278bd
63f610c
49a71a4
b41133a
e7a877b
5c213e3
d9721f1
f8387de
1c8f54c
f2c6731
4b65b3f
11e77c3
6f5ea76
cb6ce62
6d4b968
d6c957e
9d2c2f5
a17fbfa
48142fb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
storage and updater are not very descriptive. Can you add a little text here to clarify what is the base classes that are being registered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe even add a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the assert ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks super redundant.
Some of these are capitalized.
Neither distrib_updater_name nor updater_name are ever changed in any of the configurations.
Can distrib_updater_name and updater_name be properties of the trainer ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think they are best here because they are needed to instantiate the updater. But this was a mistake,
rl_hierarchical
was supposed to change these properties.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't these just be inferred from other configurations ?