Skip to content
This repository was archived by the owner on Apr 23, 2025. It is now read-only.

Address doc comments in TrainingLoop Callbacks #670

Merged
merged 4 commits into from
Oct 14, 2020
Merged

Conversation

xihui-wu
Copy link
Contributor

This change addressed the doc comments per requested in #668.

Two questions from there remains for further discussion:

  1. Use CSVLogger or CSVLog.
  2. ProgressPrinter does not need to exist as a type but:
    extension TrainingLoopProtocol {
    public mutating func printProgress(event: TrainingLoopEvent) throws { ... }
    }

@xihui-wu xihui-wu requested a review from dabrahams September 24, 2020 20:53
Copy link
Contributor

@dabrahams dabrahams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some feedback; GTG now but can give more later.

@@ -7,13 +7,13 @@ public enum CSVLoggerError: Error {

/// A handler for logging training and validation statistics to a CSV file.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't know what a handler is. Still don't think this is a strong abstraction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an observer callback where TrainingLoop is designed upon. https://docs.google.com/document/d/1CtVFhV8OcQ4E7CmNyfFeZu0IgnUPx86tfQGXiHJUXz0/edit?ts=5ebef977#heading=h.b2so9ayrnyyp

You proposed to make it a function in TrainingLoop. Here are some points I'm more in favor of making callbacks wrapped in a separate classes:

  1. Decouple from TrainingLoop
  2. Use stored properties to share callback settings
  3. Follow this pattern for all callbacks

Let's discuss more offline or in seminar meeting !?!

@@ -61,11 +60,17 @@ public class CSVLogger {
}
}

/// Writes column names to file header.
///
/// Column names include "epoch", "batch" and `stats` `name`s.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Column names include "epoch", "batch" and `stats` `name`s.
/// Column names are "epoch", "batch" and the `name` of each element of `stats`, in that order.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the ordering of dictionary elements, for a given dictionary value (its contents) is not deterministic and if this component isn't used in exactly the way you've tested it, it will be printing the wrong stats under a given column, or printing a column that contains statistic A in one row but statistic B in a second row.

I would accept an array of statistic names in the init() of this component, which determines the column order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order will preserve. I made stats to be an array of tuples instead of dictionary.

Copy link
Contributor

@dabrahams dabrahams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants