-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[change] Move hyperparameter printing entirely into StatsWriters #3630
Conversation
ml-agents/mlagents/trainers/stats.py
Outdated
def write_text(self, category: str, text: str, step: int) -> None: | ||
pass | ||
def add_property(self, category: str, key: str, value: Any) -> None: | ||
if key == "hyperparameters": |
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.
Should key
be an enum instead?
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 StatsPropertyType)
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 makes sense - less magic strings.
ml-agents/mlagents/trainers/stats.py
Outdated
@@ -23,6 +24,10 @@ def empty() -> "StatsSummary": | |||
return StatsSummary(0.0, 0.0, 0) | |||
|
|||
|
|||
class StatsPropertyType(Enum): | |||
hyperparameters = "hyperparameters" |
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.
hyperparameters = "hyperparameters" | |
HYPERPARAMETERS = "hyperparameters" |
I think generally python enums use SCREAMING_SNAKE_CASE.
Do you anticipate other parameter types being added in the future? Right now it seems a little weird to have a general proprerty interface, but only use it for hyperparameters. |
I actually took this idea from the progress bar branch, since there we need to write |
Cool, as long as they're going to get more use in the future, sounds good. |
Proposed change(s)
Hyperparameter printing was originally handled by the TrainerController and Trainer. This PR unifies the Tensorboard Hyperparameter save with the console one, and moves both into their respective ConsoleWriter and TensorboardWriter classes.
To do this, we've removed the
write_text
method from StatsWriter, which was only used to write hyperparameters (and in a TB-specific way). We now have aadd_property
method that takes in a key. This is a general way to pass data to StatsWriters, which some (or all) may choose to ignore. For instance, the hyperparameters property is a Dict and is only used by TensorboardWriter and ConsoleWriter.There are small changes to the output in the console:
OLD
NEW
Types of change(s)
Checklist