-
-
Notifications
You must be signed in to change notification settings - Fork 143
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
Feature plots description #116
Conversation
livelossplot/main_logger.py
Outdated
@@ -24,10 +24,11 @@ def __init__( | |||
current_step: int = -1, | |||
auto_generate_groups_if_not_available: bool = True, | |||
auto_generate_metric_to_name: bool = True, | |||
group_patterns: List[Tuple[str, str]] = [ | |||
group_patterns: List[Tuple[Pattern, str]] = [ | |||
(r'^(?!val(_|-))(.*)', 'training '), |
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.
Here we shouldn't have space after (e.g. 'training'
not 'training '
).
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
livelossplot/main_logger.py
Outdated
def step_name(self, group_name: str) -> str: | ||
if isinstance(self._step_names, str): | ||
return self._step_names | ||
else: | ||
return self._step_names.get(group_name, 'epoch') |
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.
It is OK, but I think it would be more idiomatic to use DefaultDict (but it can stay as it is).
self._after_subplot = after_subplot if after_subplot else self._default_after_subplot | ||
self._before_plots = before_plots if before_plots else self._default_before_plots | ||
self._after_plots = after_plots if after_plots else self._default_after_plots |
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.
Nice pattern
clear_output(wait=True) | ||
plt.figure(figsize=(figsize_x, figsize_y)) | ||
self._before_plots(len(log_groups)) |
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.
clear_output
should be in before_plots
.
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.
In general, it is nice!
A few things (except for the ones in one-line comments):
- Adding a section to README with an example (e.g. no legend, log scale)
- Did you try to use this object-oriented API (
ax.
notplt.
)?
ax.set_xlabel(x_label) | ||
ax.legend(loc='center right') | ||
|
||
def _default_before_plots(self, figure: plt.Figure, num_of_log_groups: int) -> None: |
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.
Here and in other places: it is idiomatic to use fig
not figure
.
And one more thing: in Jupyter Notebook:
|
done |
Changelist:
Notes:
step_name(group_name)
function is more comfortable to handle default value than access to a pure dictionary