-
Notifications
You must be signed in to change notification settings - Fork 416
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
Add learner training report summary #1591
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1591 +/- ##
==========================================
- Coverage 86.39% 86.35% -0.05%
==========================================
Files 688 689 +1
Lines 78675 78950 +275
==========================================
+ Hits 67974 68176 +202
- Misses 10701 10774 +73 ☔ View full report in Codecov by Sentry. |
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.
LGTM
I had one comment
Fixed your comment! Just gonna add a couple of tests for the expected results. |
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 the learner.fit(train, valid)
should return the summary with the trained model as well?
Otherwise LGTM!
I thought about that initially! Went over a couple of iterations before landing on this to retrieve the summary. Wasn't sure if we always wanted to return the summary, but it can always be added with the current implementation. |
- Add LearnerSummaryConfig - Keep track of summary metrics names - Add model field when displaying from learner.fit()
Made the requested changes we discussed offline to automatically display the summary at the end of |
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.
LGTM
Checklist
run-checks all
script has been executed.Related Issues/PRs
Closes #1264
Changes
Added
LearnerSummary
struct to retrieve recorded metrics during training.NumericEntry
enum to properly aggregate valuesNumericMetricState
to useNumericEntry
with corresponding value and batch size for each metric entry that is serializedregister_metric_train
->register_train_metric
inMetrics
to match other method namesTesting
Added aggregate test for
NumericEntry
usage with a logger and ran the examples with the newly added summary.Example Usage
Since the training artifacts are persistent (still on disk after a training is completed), the
LearnerSummary
doesn't have to be used only at the end of your training script. It can be used at any time by pointing to the artifact directory, and you can select which metrics to look for.or
Sample output:
Feel free to suggest improvements on the output formatting!