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.

Sign up for GitHub

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

Fix binary metrics so they properly return NaN when appropriate #113

Merged
merged 6 commits into from
Dec 13, 2024
Merged

Fix binary metrics so they properly return NaN when appropriate #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9c43dff
return NaNs correctly
ericphanson Dec 12, 2024
453aea7
more tests
ericphanson Dec 12, 2024
b0118e1
Update test/metrics.jl
ericphanson Dec 12, 2024
7c811ed
excludes NaNs from AUC
ericphanson Dec 12, 2024
5853125
return NaN to conform to schema
ericphanson Dec 12, 2024
7cfce07
tests
ericphanson Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name = "Lighthouse"
uuid = "ac2c24cd-07f0-4848-96b2-1b82c3ea0e59"
authors = ["Beacon Biosignals, Inc."]
version = "0.17.1"
version = "0.17.2"

[deps]
ArrowTypes = "31f734f8-188a-4ce0-8406-c8a06bd891cd"
Expand Down
19 changes: 5 additions & 14 deletions src/metrics.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,20 +73,11 @@ function binary_statistics(confusion::AbstractMatrix, class_index::Integer)
false_positives = predicted_positives - true_positives
false_negatives = actual_positives - true_positives
true_negatives = actual_negatives - false_positives
true_positive_rate = (true_positives == 0 && actual_positives == 0) ?
(one(true_positives) / one(actual_positives)) :
(true_positives / actual_positives)
true_negative_rate = (true_negatives == 0 && actual_negatives == 0) ?
(one(true_negatives) / one(actual_negatives)) :
(true_negatives / actual_negatives)
false_positive_rate = (false_positives == 0 && actual_negatives == 0) ?
(zero(false_positives) / one(actual_negatives)) :
(false_positives / actual_negatives)
false_negative_rate = (false_negatives == 0 && actual_positives == 0) ?
(zero(false_negatives) / one(actual_positives)) :
(false_negatives / actual_positives)
Comment on lines -76 to -87
Copy link

Choose a reason for hiding this comment

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

Do you remember there a rationale behind these quantities being 1/0 in the edge case? I agree these should return NaN but just curious

Copy link
Member Author

Choose a reason for hiding this comment

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

I traced this code back to https://github.com/beacon-biosignals/OldLighthouse.jl/pull/36 (private repo), so I believe it was to support ROC curves better, where NaNs could contaminate the whole AUC even if it's just some 0.0 threshold or something. But IMO that is much better handled in the AUC computation itself rather than here.

precision = (true_positives == 0 && predicted_positives == 0) ? NaN :
(true_positives / predicted_positives)
true_positive_rate = true_positives / actual_positives
true_negative_rate = true_negatives / actual_negatives
false_positive_rate = false_positives / actual_negatives
false_negative_rate = false_negatives / actual_positives
precision = true_positives / predicted_positives
f1 = true_positives / (true_positives + 0.5 * (false_positives + false_negatives))
return (; predicted_positives, predicted_negatives, actual_positives, actual_negatives,
true_positives, true_negatives, false_positives, false_negatives,
Expand Down
8 changes: 6 additions & 2 deletions src/utilities.jl
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

Do you know the rationale behind lighthouse returning missing instead of NaN? I'm worried that there may be code that depends on this on this behavior rather than returning NaN.

Copy link
Member Author

@ericphanson ericphanson Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, and I agree this is a dangerous change. However, the most common usages of area_under_curve are

Lighthouse.jl/src/metrics.jl

Line 222 in 2a4494e

return TradeoffMetricsV1(; class_index, class_labels, roc_curve,
which does not support missing:

Lighthouse.jl/src/row.jl

Line 302 in 2a4494e

roc_auc::Float64

So I think there is a bug somewhere, either TradeoffMetricsV1 needs to support missing or area_under_curve needs to not return missing. I believe this change will be less disruptive. It does contravene the docstring for area_under_curve so I believe it is technically a breaking change. We could do a breaking version bump here but that will cause a lot of compat updating work and I don't think this will break any users in practice, and it is essentially a bugfix, so I think it's OK. But it is a bit dicey.

Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,15 @@ end
area_under_curve(x, y)

Calculates the area under the curve specified by the `x` vector and `y` vector
using the trapezoidal rule. If inputs are empty, return `missing`.
using the trapezoidal rule. If inputs are empty, return `NaN`. Excludes NaN entries.
"""
function area_under_curve(x, y)
length(x) == length(y) || throw(ArgumentError("Length of inputs must match."))
length(x) == 0 && return missing
isempty(x) && return NaN
non_nan = (!).(isnan.(x) .| isnan.(y))
x = x[non_nan]
y = y[non_nan]
isempty(x) && return NaN
Copy link

Choose a reason for hiding this comment

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

do we need to check if y is empty as well?

Copy link

Choose a reason for hiding this comment

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

Nvm, I was confusing myself initially but I think the single check covers it! If x is empty y must be empty as well, so the single check is sufficient.

auc = zero(middle(one(eltype(x)), one(eltype(y))))
perms = sortperm(x)
sorted_x = view(x, perms)
Expand Down
13 changes: 8 additions & 5 deletions test/metrics.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,10 @@
@test stats.true_negatives == 0
@test stats.false_positives == 0
@test stats.false_negatives == 0
@test stats.true_positive_rate == 1
@test stats.true_negative_rate == 1
@test stats.false_positive_rate == 0
@test stats.false_negative_rate == 0
@test isnan(stats.true_positive_rate)
@test isnan(stats.true_negative_rate)
@test isnan(stats.false_positive_rate)
@test isnan(stats.false_negative_rate)
@test isnan(stats.precision)
@test isnan(stats.f1)

Expand All @@ -100,6 +100,8 @@
@test stats.false_positives == 0
@test stats.false_negatives == 0
@test isnan(stats.f1)
@test isnan(stats.true_positive_rate)
@test isnan(stats.false_negative_rate)

c = [0 2
0 6]
Expand All @@ -109,6 +111,8 @@
@test stats.false_positives == 2
@test stats.false_negatives == 0
@test stats.f1 == 0
@test isnan(stats.true_positive_rate)
@test isnan(stats.false_negative_rate)

c = [0 0
2 6]
Expand All @@ -118,7 +122,6 @@
@test stats.false_positives == 0
@test stats.false_negatives == 2
@test stats.f1 == 0

for p in 0:0.1:1
@test Lighthouse._cohens_kappa(p, p) == 0
if p > 0
Expand Down
6 changes: 4 additions & 2 deletions test/utilities.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ end

@testset "`Lighthouse.area_under_curve`" begin
@test_throws ArgumentError Lighthouse.area_under_curve([0, 1, 2], [0, 1])
@test ismissing(Lighthouse.area_under_curve([], []))
@test isnan(Lighthouse.area_under_curve([], []))
@test isnan(Lighthouse.area_under_curve([NaN], [NaN]))
@test isapprox(Lighthouse.area_under_curve(collect(0:0.01:1), collect(0:0.01:1)), 0.5;
atol=0.01)
@test isapprox(Lighthouse.area_under_curve(collect(0:0.01:(2π)), sin.(0:0.01:(2π))),
Expand All @@ -17,7 +18,8 @@ end

@testset "`Lighthouse.area_under_curve_unit_square`" begin
@test_throws ArgumentError Lighthouse.area_under_curve_unit_square([0, 1, 2], [0, 1])
@test ismissing(Lighthouse.area_under_curve_unit_square([], []))
@test isnan(Lighthouse.area_under_curve_unit_square([], []))
@test isnan(Lighthouse.area_under_curve_unit_square([NaN], [NaN]))
@test isapprox(Lighthouse.area_under_curve_unit_square(collect(0:0.01:1),
collect(0:0.01:1)), 0.5;
atol=0.01)
Expand Down
Loading