Skip to content
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

Update log_evaluation_row! to not special case Spearman correlation coefficient #64

Open
ericphanson opened this issue May 3, 2022 · 3 comments

Comments

@ericphanson
Copy link
Member

okay, i'm not "allowed" to comment below this point, but I think we should specialize the below implementation of
log_evaluation_row! on LearnLogger so that special-casing spearman correlation doesn't have to happen by default---that is a very tb-specific logged item. E.g.,

function log_evaluation_row!(logger, field::AbstractString, metrics)
    metrics_plot = evaluation_metrics_plot(metrics)
    metrics_dict = _evaluation_row_dict(metrics)
    log_plot!(logger, field, metrics_plot, metrics_dict)
    return metrics_plot
end

or, better:

function log_evaluation_row!(logger, field::AbstractString, metrics)
    metrics_plot = evaluation_metrics_plot(metrics)
    log_plot!(logger, field, metrics_plot) # if we make the plot_data field optional
    # optionally, could also then log each field of `metrics_plot` with `log_values!`
    return metrics_plot
end

and

function log_evaluation_row!(logger::LearnLogger, field::AbstractString, metrics)
    metrics_plot = evaluation_metrics_plot(metrics)
    metrics_dict = _evaluation_row_dict(metrics)
    log_plot!(logger, field, metrics_plot, metrics_dict)
    if haskey(metrics_dict, "spearman_correlation")
        sp_field = replace(field, "metrics" => "spearman_correlation")
        log_value!(logger, sp_field, metrics_dict["spearman_correlation"].ρ)
    end
    return metrics_plot
end

Originally posted by @hannahilea in #60 (comment)

@ericphanson
Copy link
Member Author

I think we could also consider changing log_evaluation_row to also call log_line_series! to log out the ROC and PR curves.

@hannahilea
Copy link
Contributor

hannahilea commented May 3, 2022

And the follow-up:

I don't really get why spearman is TB-specific; shouldn't either all loggers want it or none? @ericphanson

So the history here is, back in the VERY beginning we wanted to track all of our Lighthouse metrics in tensorboard. And (at the time, potentially not still now) it was tricky (and not an internal priority) to do things like natively log line series or histograms or any of the values that Lighthouse was calculating. So instead, we logged the image of the plots generated from those metrics directly. Then, later, we wanted to be able to directly compare spearman correlation across runs from within tb, and since that was a single value, it was easy to use tb-native logging for it---so we did, and decided that any other values like that could be added as needed. (In hindsight, that special casing should have been done outside this package, but c'est la vie!)

In reality, spearman is no more special/necessary/useful than any of the other metrics, and callers will likely want to log and be able to track most/all of them if they want any of them. What should the default behavior be, when not designed for tb? Probably some combo of "posting all fields in the metrics" (which currently include both scalar values and line series) and/or posting the image. (In the (not yet public) LWB we do both image and scalars, and should---but don't yet---log the line series as well)

@ericphanson
Copy link
Member Author

Makes sense! Yeah, I think we should just log everything here (scalars, curves, etc), dispatching to log_values! and log_line_series! etc.

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

No branches or pull requests

2 participants