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

Feature/sensitivity low alert #2001

Merged
merged 35 commits into from Mar 29, 2021

Conversation

skvorekn
Copy link
Contributor

@skvorekn skvorekn commented Mar 21, 2021

Sensitivity at low alert rates is an evaluation metric used in the final round of the Centers for Medicare & Medicaid Services AI Health Outcomes Challenge.

It is an important clinical accuracy metric because it gives insight into how resource reallocation will impact outcomes. For example, if we are predicting mortality of the Medicare population, a hospital may want to allocate more resources to people with the top, say, 1% of risk scores (predictions). This 1% is the alert rate. As a measure of accuracy, the hospital may want to know what percent of all deaths are captured by focusing on the top 1% (sensitivity).

The sensitivity metric is calculated by classifying the top 1% of predictions as the 'True' class (we predict they will die), and the remaining 99% are classified as 'False' (we predict they will not die). Sensitivity is measured using this alert rate (# predicted true/# actual true).

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Mar 21, 2021

Codecov Report

Merging #2001 (14e80ed) into main (20ddcd6) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             main    #2001     +/-   ##
=========================================
+ Coverage   100.0%   100.0%   +0.1%     
=========================================
  Files         278      280      +2     
  Lines       22770    22886    +116     
=========================================
+ Hits        22761    22877    +116     
  Misses          9        9             
Impacted Files Coverage Δ
evalml/objectives/__init__.py 100.0% <100.0%> (ø)
evalml/objectives/sensitivity_low_alert.py 100.0% <100.0%> (ø)
...tive_tests/test_binary_classification_objective.py 100.0% <100.0%> (ø)
evalml/tests/objective_tests/test_objectives.py 100.0% <100.0%> (ø)
evalml/tests/objective_tests/test_sla.py 100.0% <100.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20ddcd6...14e80ed. Read the comment docs.

@skvorekn skvorekn marked this pull request as ready for review March 21, 2021 19:12
@skvorekn
Copy link
Contributor Author

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ skvorekn
❌ Kelsey Skvoretz

Kelsey Skvoretz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Please advise on how to resolve this. I only have one GitHub account, so not sure what is going on here! Thanks!

@dsherry
Copy link
Contributor

dsherry commented Mar 22, 2021

Hi @skvorekn , thanks for contributing! Regarding your question about the CLA, it appears the email you've made your commits with doesn't match one of the emails in your GitHub account. Here's a guide from GitHub on how to set your email address on the GitHub site, including a section on how to set it on the command-line so git will use it.

My recommendation would be to set this to your GitHub account email and then squash your changes:

git config --global user.email "email@example.com"
# squash your branch down to one commit. instructions here: https://stackoverflow.com/a/5189600/841003
git fetch && git rebase -i origin/main
git push -f

If that doesn't work please let us know.

Copy link
Collaborator

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

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

Solid. I like this PR and the testing framework. Thanks a lot for the contribution!

evalml/objectives/sensitivity_low_alert.py Outdated Show resolved Hide resolved
evalml/objectives/sensitivity_low_alert.py Outdated Show resolved Hide resolved
evalml/tests/objective_tests/test_sla.py Show resolved Hide resolved
@skvorekn skvorekn force-pushed the feature/sensitivity-low-alert branch from 5e79138 to 808ab3a Compare March 22, 2021 22:13
@skvorekn
Copy link
Contributor Author

I misunderstood the comment about squashing changes, so did that out of order. Looks like things are passing now, though! Also made a few minor updates based on @chukarsten's comments. Thanks for your feedback - I really appreciate it! Let me know if there is anything else.

Copy link
Contributor

@dsherry dsherry left a comment

Choose a reason for hiding this comment

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

This is wonderful! The code looks great. My only comment was about the class name.

evalml/objectives/sensitivity_low_alert.py Outdated Show resolved Hide resolved
@chukarsten
Copy link
Collaborator

chukarsten commented Mar 26, 2021

@skvorekn I think you might need to resign our CLA to pass our CI check. You were this close 🤏 to making it into the last release . I also rebased your remote branch so you'll probably have to pull.

docs: update api reference with SLA

refactor: X is not a used argument in objective function

feat: initialize SLA as an objective

feat: raise error if alert rate is invalid

feat: binary objective test base class

test: sla using base class

refactor: define data in run_pipeline

test: class flags match expected given list of predictions and alert rate

style: rename object to obj

test: expected sensitivity score

chore: remove commented out get_data fn

style: rename for abstract method

feat: test all binary base class tests with wrapper test

style: linting fixes

refactor: scope of fixtures is within class

fix: wrong exception raised

fix: rename base class tests to methods so test doesn't run

refactor: move binary base class to existing file

test: 8 binary problem types

Revert "docs: update api reference with SLA"

This reverts commit e26bf77.

chore: rebase with main

style: sort imports

fix: remove X argument from docstring

feat: Exception to ValueError

feat: Exception to ValueError
@skvorekn skvorekn force-pushed the feature/sensitivity-low-alert branch from 5fb07e0 to d2b5c42 Compare March 29, 2021 16:28
@skvorekn
Copy link
Contributor Author

Done! Looks like the CLA needs a resign by @chukarsten as well!

@chukarsten chukarsten merged commit eb008b3 into alteryx:main Mar 29, 2021
@chukarsten
Copy link
Collaborator

@skvorekn merged! We'll release it next week with 0.22.0.

@chukarsten chukarsten mentioned this pull request Apr 6, 2021
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

Successfully merging this pull request may close these issues.

None yet

4 participants