Christian/update estimate prioritization #268
Conversation
…to match SOP; Reordered "Test Type" prioritization to match SOP Adjustment: - Changed order to (only Pop Adj > only Test Adj) Test types: -Added 'Multiple Types' underneath 'Neutralization'. -Reorganized single test type options (ELISA > CLIA)
123chrisc
requested review from
annoyingpr,
atatmaja,
rahularoradfs and
simonarocco
| context: . | ||
| dockerfile: Dockerfile-app | ||
| restart: always | ||
| image: "iit-backend/app" |
There was a problem hiding this comment.
we don't want to actually push this change so don't forget to revert this
There was a problem hiding this comment.
Sorry for late follow-up on this. Did not open GitHub for a hot minute. Is this change sorted or do I need to go back and reverse it?
There was a problem hiding this comment.
@123chrisc you need to undo this change, i.e. put this line back please.
There was a problem hiding this comment.
@123chrisc, congratulations on the pull request!! Overall looks good to me. I do have a few broader questions before we go ahead and approve this.
Things that should be done here:
- Did you check to ensure that all variable names are correct here?
- Have you tested to ensure that this doesn't break any functionality yourself, has a dev team member done so, or did @atatmaja indicate that this wasn't actually necessary with our automated tests?
Things that don't necessarily need to be part of this PR, but need to be done by the time we get the meta-analysis in:
- Did you check to ensure that all variables here map to the correct variables in the Airtable? This is a check we should do in general to ensure that all Airtable changes have actually been propagated to the database. Here is the file you need to check to ensure that: https://github.com/serotracker/iit-backend/blob/master/app/utils/airtable_fields_config.py . In this file, focus on the
full_airtable_fieldsmapping; things before colon are Airtable names from estimates field; after colon are names in the database - Have you checked to ensure that every variable has a pooling function in https://github.com/serotracker/iit-backend/blob/master/app/utils/estimate_prioritization/pooling_functions.py ?
Would be great if you could share a plan for the last two, whether or not they're part of this PR :)
| lambda estimate: (estimate['pop_adj'] is True) and (estimate['test_adj'] is True), | ||
| lambda estimate: (pd.isna(estimate['pop_adj'])) and (estimate['test_adj'] is True), | ||
| lambda estimate: (estimate['pop_adj'] is True) and (pd.isna(estimate['test_adj'])), | ||
| lambda estimate: (pd.isna(estimate['pop_adj'])) and (estimate['test_adj'] is True), |
There was a problem hiding this comment.
Seems fair to me @123chrisc - could certainly make a cogent argument that now with higher seroprevalence and better assays, we should prioritize pop-adj test-unadj in these cases
There was a problem hiding this comment.
Variables were all checked to match. Sorting pooling functions now!
There was a problem hiding this comment.
All variables do have a pooling function. They are also matched to the airtable_fields_config.py
| lambda estimate: estimate['test_type'] == 'CLIA', | ||
| lambda estimate: estimate['test_type'] == 'ELISA' | ||
| lambda estimate: estimate['test_type'] == 'LFIA', | ||
| lambda estimate: estimate['test_type'] == 'Other', |
There was a problem hiding this comment.
These 'catch-call' cases are already implied to be the last thing in the hierarchy, if they're not listed among the options listed- but it's good to be explicit about them!
| context: . | ||
| dockerfile: Dockerfile-app | ||
| restart: always | ||
| image: "iit-backend/app" |
|
Can we get this merged @123chrisc @simonarocco? |
@123chrisc just an FYI, as the PR request owner, it is your responsibility to get the PR merged. this means getting the approvals that you need to merge the PR (right now you have zero approvals). once you get these approvals you can merge. i believe there are still some unaddressed comments. once you have addressed a reviewer's comment please "resolve" the comment |
simonarocco
left a comment
simonarocco
left a comment
There was a problem hiding this comment.
this looks good to me, but you need to revert the change to the docker-compose.yml file. also, please get rahul's approval on the changes to estimate prioritization before merging this
| context: . | ||
| dockerfile: Dockerfile-app | ||
| restart: always | ||
| image: "iit-backend/app" |
There was a problem hiding this comment.
@123chrisc you need to undo this change, i.e. put this line back please.
|
Thanks! @simonarocco: I left a comment above re: what needs to be done before merging. Copying that below
|
Briefly describe the feature or bug that this PR addresses.
This PR addresses (slightly) outdated estimate prioritization code
Please link the Airtable ticket associated with this PR.
N/A?
Describe the steps you took to test the feature/bugfix introduced by this PR.
I reviewed the estimate prioritization code and compared with our SOP and protocol prioritization. After making comparisons, I made minor edits to match.
Does any infrastructure work need to be done before this PR can be pushed to production?
No?