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

clarify use of enw_retrospective_data #82

Merged
merged 14 commits into from
Jul 6, 2022
Merged

Conversation

sbfnk
Copy link
Collaborator

@sbfnk sbfnk commented Jun 29, 2022

Aims to fix #79 with documentation and clearer argument names.

R/preprocess.R Outdated Show resolved Hide resolved
@seabbs seabbs mentioned this pull request Jul 4, 2022
21 tasks
@seabbs
Copy link
Collaborator

seabbs commented Jul 4, 2022

So I looked at this a bit more and thought that maybe it would be better if enw_retrospective_data was split into two functions and standardised with enw_latest_data

Doing this gives the following workflow which personally I think is a little clearer and easier to use (much easier if allowing yourself to use some kind of pipe).

# Load epinowcast and data.table
library(epinowcast)
library(data.table)

# Load and filter germany hospitalisations
nat_germany_hosp <- germany_covid19_hosp[location == "DE"][age_group %in% "00+"]
nat_germany_hosp <- nat_germany_hosp[report_date <= as.Date("2021-10-01")]

# Make a retrospective dataset
retro_nat_germany <- nat_germany_hosp |>
  enw_filter_report_dates(remove_days = 40) |> 
  enw_filter_reference_dates(include_days = 40)

# Get latest observations for the same time period
latest_obs <- nat_germany_hosp |>
  enw_latest_data() |>
  enw_filter_report_dates(remove_days = 40) |> 
  enw_filter_reference_dates(include_days = 40)

The potential downside here is the duplication for retrospective and latest data but I think that might be hard to avoid.

Note: I haven't followed through fully with this idea yet (i.e tests, documentation updates, etc.) to avoid wasting work if we don't stick with this. Also yes this should probably be happening in another PR etc. etc. but sometimes we all have more ice cream than we should.

@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #82 (aca7847) into develop (68765e2) will increase coverage by 0.95%.
The diff coverage is 100.00%.

❗ Current head aca7847 differs from pull request most recent head 7f1a325. Consider uploading reports for the commit 7f1a325 to get more accurate results

@@             Coverage Diff             @@
##           develop      #82      +/-   ##
===========================================
+ Coverage    55.92%   56.88%   +0.95%     
===========================================
  Files           12       12              
  Lines          928      937       +9     
===========================================
+ Hits           519      533      +14     
+ Misses         409      404       -5     
Impacted Files Coverage Δ
R/formula-tools.R 92.14% <ø> (ø)
R/check.R 68.42% <100.00%> (+68.42%) ⬆️
R/preprocess.R 94.38% <100.00%> (+2.49%) ⬆️
R/utils.R 65.71% <100.00%> (+3.21%) ⬆️

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 68765e2...7f1a325. Read the comment docs.

@seabbs seabbs merged commit 93218fd into develop Jul 6, 2022
@seabbs seabbs deleted the clarify-retrospective branch July 6, 2022 14:31
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

2 participants