-
Notifications
You must be signed in to change notification settings - Fork 2
add logging interface #60
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
Changes from 3 commits
6f2962e
86299ee
79a15c7
8935c90
341181e
635d3b7
fda20fb
b771a6d
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -24,19 +24,45 @@ function LearnLogger(path, run_name; kwargs...) | |||||
| return LearnLogger(path, tensorboard_logger, Dict{String,Any}()) | ||||||
| end | ||||||
|
|
||||||
| """ | ||||||
| log_event!(logger, value::AbstractString) | ||||||
|
|
||||||
| Logs a string event given by `value` to `logger`. | ||||||
| """ | ||||||
| log_event!(logger, value) | ||||||
|
|
||||||
| function log_event!(logger::LearnLogger, value) | ||||||
|
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. Hrm, maybe we should move all of the LearnLogger-specific functions to their own file? Would make it extra clear what is specific to
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. My thought is maybe we could do something like function log_event!(logger, value)Because LWB is dispatching this for a different type of logger, and it makes sense for lighthouse to provide the fallback methods for the different logging actions
Member
Author
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. added fallback! |
||||||
| logged = string(now(), " | ", value) | ||||||
| TensorBoardLogger.log_text(logger.tensorboard_logger, "events", logged) | ||||||
| return logged | ||||||
| end | ||||||
|
|
||||||
| """ | ||||||
| log_plot!(logger, field::AbstractString, plot, plot_data) | ||||||
|
|
||||||
| Log a `plot` to `logger` under field `field`. | ||||||
|
|
||||||
| * `plot`: the plot itself | ||||||
| * `plot_data`: an unstructured dictionary of values used in creating `plot`. | ||||||
|
|
||||||
| See also [`log_line_series!`](@ref). | ||||||
| """ | ||||||
| log_plot!(logger, field::AbstractString, plot, plot_data) | ||||||
|
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. ...I really really wish we didn't have to log both the plot and the data in this function; that was an addition that we made as a tb workaround (we were plotting an image of the plotted data, but weren't otherwise serializing the data used to do the plotting) and in hindsight that should have been a separate function call. If there's a way to back out of it now, we should consider it...could this be, e.g., an
Suggested change
Member
Author
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. I think this runs the risk of switching loggers and then existing code breaking (since it relies on being able to pass some number of args); ideally, you should be able to switch loggers without any code changes. I agree about
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. yeah I agree with this, especially as we build out more plot logging functionalities we will probably want to deprecate
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. Yeah, sounds good to me--- |
||||||
|
|
||||||
| function log_plot!(logger::LearnLogger, field::AbstractString, plot, plot_data) | ||||||
| values = get!(() -> Any[], logger.logged, field) | ||||||
| push!(values, plot_data) | ||||||
| TensorBoardLogger.log_image(logger.tensorboard_logger, field, plot; step=length(values)) | ||||||
| return plot | ||||||
| end | ||||||
|
|
||||||
| """ | ||||||
| log_plot!(logger, field::AbstractString, value) | ||||||
|
ericphanson marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| Log a scalar value `value` to `field`. | ||||||
|
Member
Author
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. Not sure if this has to be a scalar value? I think what it supports might depend on the logging backend...
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. yeah, I think it is fair to remove the term "scalar" here, since it is a plot, not sure if we can consider it a scalar or not!
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. ah rename to log_value!, again I do not think the word scalar allows us the flexibility we will probably have, we should update the LighthouseWandb docs accordingly as well
Member
Author
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. I removed the word
ericphanson marked this conversation as resolved.
Outdated
|
||||||
| """ | ||||||
| log_value!(logger, field::AbstractString, value) | ||||||
|
|
||||||
| function log_value!(logger::LearnLogger, field::AbstractString, value) | ||||||
| values = get!(() -> Any[], logger.logged, field) | ||||||
| push!(values, value) | ||||||
|
|
@@ -45,12 +71,29 @@ function log_value!(logger::LearnLogger, field::AbstractString, value) | |||||
| return value | ||||||
| end | ||||||
|
|
||||||
| """ | ||||||
| log_line_series!(logger, field::AbstractString, curves, labels=1:length(curves)) | ||||||
|
|
||||||
| Logs a series plot to `logger` under `field`, where... | ||||||
|
|
||||||
| - `curves` is an iterable of the form `Tuple{Vector{Real},Vector{Real}}`, where each tuple contains `(x-values, y-values)`, as in the `Lighthouse.EvaluationRow` field `per_class_roc_curves` | ||||||
| - `labels` is the class label for each curve, which defaults to the numeric index of each curve. | ||||||
| """ | ||||||
| log_line_series!(logger, field::AbstractString, curves, labels=1:length(curves)) | ||||||
|
|
||||||
| function log_line_series!(logger::LearnLogger, field::AbstractString, series, series_labels) | ||||||
| @warn "`log_line_series!` not implemented for `LearnLogger`" | ||||||
| function log_line_series!(logger::LearnLogger, field::AbstractString, curves, labels=1:length(curves)) | ||||||
| @warn "`log_line_series!` not implemented for `LearnLogger`" maxlog=1 | ||||||
| return nothing | ||||||
| end | ||||||
|
|
||||||
| """ | ||||||
| step_logger!(logger) | ||||||
|
|
||||||
| Increments the `logger`'s `step`, if any. Defaults to doing nothing. | ||||||
| """ | ||||||
| step_logger!(logger) = nothing | ||||||
|
|
||||||
|
|
||||||
| """ | ||||||
|
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. okay, i'm not "allowed" to comment below this point, but I think we should specialize the below implementation of or, better: and
Member
Author
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. I don't really get why spearman is TB-specific; shouldn't either all loggers want it or none?
Member
Author
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. I filed #64 to track this |
||||||
| log_evaluation_row!(logger, field::AbstractString, metrics) | ||||||
|
|
||||||
|
|
@@ -186,7 +229,7 @@ end | |||||
| evaluate!(predicted_hard_labels::AbstractVector, | ||||||
| predicted_soft_labels::AbstractMatrix, | ||||||
| elected_hard_labels::AbstractVector, | ||||||
| classes, logger::LearnLogger; | ||||||
| classes, logger; | ||||||
| logger_prefix, logger_suffix, | ||||||
| votes::Union{Nothing,AbstractMatrix}=nothing, | ||||||
| thresholds=0.0:0.01:1.0, | ||||||
|
|
@@ -842,6 +885,7 @@ end | |||||
| ##### | ||||||
|
|
||||||
| """ | ||||||
| upon(logger::LearnLogger, field::AbstractString; condition, initial) | ||||||
| upon(logged::Dict{String,Any}, field::AbstractString; condition, initial) | ||||||
|
|
||||||
| Return a closure that can be called to check the most recent state of | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.