-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix rl config #50
Fix rl config #50
Conversation
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.
Seems fine, I guess it was mostly linting outside of cleaning up the configs?
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.
Not sure if you use this it all, but you can probably delete this module.
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.
yah mostly. There were some things I wanted to look into / compare, but maybe I can add @ deprecated for now already
(rest of this will be addressed in #35 )
More or less. Actually since you changed the constructor of Brain I had to make several changes down the line, basically whereever I'm interfacing to sample factory to use the brain... also you had a check whether the config contains an optimizer which I removed. Are there cases where a config can be created without an optimizer? In any case, the config processing should be separated from the main function imho (opened a ticket for that #52) |
Finally got the RL config working again
added a working config to the examples
made necessary changes to the rl framework + related modules to adapt to config changes
most notably I needed a function building the Brain from a DictConfig for better interfacing (and in order to not duplicate code)