Proposed fix for reporting and missing column unexpected behavior #150
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hjohns12
reviewed
Mar 16, 2022
R/calc_aggregate_counts.R
Outdated
| # Aggregate by Val, including MP, then add the reporting and missing | ||
| out_agg_df <- agg_df %>% | ||
| summarize(Count = sum_na_rm(Val)) %>% | ||
| merge(UCLA_reporting_and_missing) |
There was a problem hiding this comment.
Tiny suggestion -- if you use left_join here rather than merge, the object created is a tibble which makes things print more nicely in the terminal! Not necessary but might be nice to have
|
This looks great! The only change I noticed and didn't expect to see was |
|
Thanks for your feedback Hope! I incorporated the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue 149
This code could be merged on its own to make the 'reported' and 'missing' columns refer to what was reported and missing for UCLA, rather than the current behavior. The current behavior counts a state as reporting if there is either data for UCLA or data from the Marshall Project from June. However...
If we move forward with removing references to the Marshall Project, the desired behavior (reported and missing both refer to UCLA's access to data) will happen on its own. OR if we move the global cutoff date to December 2021, then the behavior goes away on its own. BUT both options here will lower the cumulative counts substantially. I have to redo my comparison to be specific, but one thing I remember is that deaths go down by 233, for instance.
I think the least disruptive change to our data users would be to have the max ever cumulative figures available before either changing the cutoff date or removing the Marshall Project. But that's totally different work whose time requirement we don't know. So I think in the end, it may be best to merge this patch, but there's a good argument to be made for just dealing with the hit to cumulative figures too. Food for thought.