Open ericphanson opened 2 years ago
I think we could also consider changing log_evaluation_row
to also call log_line_series!
to log out the ROC and PR curves.
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)
Makes sense! Yeah, I think we should just log everything here (scalars, curves, etc), dispatching to log_values!
and log_line_series!
etc.
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.,or, better:
and
_Originally posted by @hannahilea in https://github.com/beacon-biosignals/Lighthouse.jl/pull/60#discussion_r863082625_