-
Notifications
You must be signed in to change notification settings - Fork 146
Added metrics docstrings #1392
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
base: main
Are you sure you want to change the base?
Added metrics docstrings #1392
Changes from 3 commits
b22eea7
34095e4
a346dd0
67e0062
3017b11
bfaac3f
4ee672f
43b4de4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| # Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| # All rights reserved. | ||
| # | ||
|
|
@@ -15,17 +15,27 @@ | |
|
|
||
| @final | ||
| class CompositeMetricRecorder(MetricRecorder): | ||
| """ | ||
| Represents a collection defining a recorder for multiple metrics | ||
| """ | ||
| def __init__(self, recorders: Collection[MetricRecorder]) -> None: | ||
| self._recorders = recorders | ||
|
|
||
| @override | ||
| def record_metric_values( | ||
| self, category: str, values: Mapping[str, object], step_nr: int | None = None | ||
| ) -> None: | ||
| """ | ||
| Iterates through recorders for all metrics and records the values | ||
| For each metric type, the category, values, and step number are recorded | ||
| """ | ||
| for recorder in self._recorders: | ||
| recorder.record_metric_values(category, values, step_nr) | ||
|
|
||
| @override | ||
| def close(self) -> None: | ||
| """ | ||
| Closes and removes the :class:MetricRecorder | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
| """ | ||
| for recorder in self._recorders: | ||
| recorder.close() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,11 @@ def __init__( | |
| def record_metric_values( | ||
| self, category: str, values: Mapping[str, object], step_nr: int | None = None | ||
| ) -> None: | ||
| """ | ||
| Gets, sorts, and maps metrics, their values, and descriptions to a ``dict`` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, no need to override the docstr of base interface. Any behavior specific to this subclass should ideally be described in the class docstr. |
||
| as a stream. Dumps output to a json file. | ||
| :raises OSError: If unable to write to file | ||
| """ | ||
| stream = self._get_stream(category) | ||
|
|
||
| values_and_descriptors = [] | ||
|
|
@@ -69,6 +74,10 @@ def record_metric_values( | |
| values_and_descriptors.sort(key=lambda p: (p[1].priority, p[1].display_name)) | ||
|
|
||
| def sanitize(value: object) -> object: | ||
| """ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an inline function. No need for docstr. |
||
| Sanitizes `value` by enforcing object type | ||
| :raise ValueError: If `value` is not of type ``int``, ``float``, ``Tensor``, ``str`` | ||
| """ | ||
| if isinstance(value, Tensor): | ||
| if value.numel() != 1: | ||
| return value.tolist() | ||
|
|
@@ -103,6 +112,11 @@ def sanitize(value: object) -> object: | |
| raise_operational_system_error(ex) | ||
|
|
||
| def _get_stream(self, category: str) -> TextIO: | ||
| """ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to document private functions. |
||
| Opens a stream for a given category | ||
| :raise ValueError: If regex catches nonalphnumeric chars or dash, underscore, forward slash | ||
| :raise OSError: If an operating system error occurs when making directory or creating a file | ||
| """ | ||
| category = category.strip() | ||
|
|
||
| fp = self._streams.get(category) | ||
|
|
@@ -133,6 +147,9 @@ def _get_stream(self, category: str) -> TextIO: | |
|
|
||
| @override | ||
| def close(self) -> None: | ||
| """ | ||
| Closes the stream and clears object | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
| """ | ||
| for stream in self._streams.values(): | ||
| stream.close() | ||
|
|
||
|
|
||
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.
I don't think we need to add documentation for overridden methods. The docstr of the method on the corresponding interface (in this case
MetricRecorder) should be descriptive enough to explain the expected behavior of the metod.