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

Jump to bottom

jidea-110 New time series #30

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
+110 −2
Open

jidea-110 New time series #30

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
640d6fa
add new time series
EmmaLRussell Oct 21, 2024
7d7d610
time series metadata
EmmaLRussell Oct 21, 2024
9e01b1d
add metadata
EmmaLRussell Oct 21, 2024
1276b19
unit test
EmmaLRussell Oct 21, 2024
b1442da
fix schema
EmmaLRussell Oct 21, 2024
c8bd90a
test get_incidence calls
EmmaLRussell Oct 21, 2024
5bdcc2d
lint
EmmaLRussell Oct 21, 2024
c66e074
more lint
EmmaLRussell Oct 21, 2024
aa1d4a5
call get_incidence once only
EmmaLRussell Oct 21, 2024
6286cda
update time series names
EmmaLRussell Oct 21, 2024
c601838
update new hosp description
EmmaLRussell Oct 21, 2024
140b7cc
shorten long line
EmmaLRussell Oct 21, 2024
4c793d1
update e2e test
EmmaLRussell Oct 21, 2024
3f5e97b
lint
EmmaLRussell Oct 21, 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
10 changes: 10 additions & 0 deletions R/model_run.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ model_run <- function(parameters, model_version) {
)
time_series$vaccinated <- vax_time_series$vaccinated

# get incidence time series
time_series$new_infections <-
daedalus::get_incidence(model_results, "infections")$value
time_series$new_hospitalisations <-
daedalus::get_incidence(model_results, "hospitalisations")$value
time_series$new_deaths <-
daedalus::get_incidence(model_results, "deaths")$value
time_series$new_vaccinations <-
daedalus::get_new_vaccinations(model_results)$new_vaccinations
Copy link
Contributor

Choose a reason for hiding this comment

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

The code here definitely works, but here's a suggestion to cut down calls to get_incidence():

model_results = daedalus("GBR", "sars_cov_1")

# already has all three time series in long format
incidences = get_incidence(output)

# pivot to wide format
incidences_wide = tidyr::pivot_wider(
  incidences, id_cols = "time", names_from = "measure"
)

# get timeseries, e.g.
time_series$new_infections = incidences_wider$daily_infections


raw_costs <- daedalus::get_costs(model_results)
costs <- get_nested_costs(raw_costs)

Expand Down
44 changes: 43 additions & 1 deletion inst/json/metadata_0.1.0.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,49 @@
"description": "Number of infectious individuals, comprising both symptomatic and asymptomatic infections, including those in need of hospitalisation." },
{ "id": "hospitalised", "label": "Hospital demand", "description": "Infections requiring hospitalisation" },
{ "id": "dead", "label": "Dead", "description": "Total deaths" },
{ "id": "vaccinated", "label": "Vaccinated", "description": "Total number of vaccinations administered" }
{ "id": "vaccinated", "label": "Vaccinated", "description": "Total number of vaccinations administered" },
{ "id": "new_infections", "label": "New Infections", "description": "Number of new infections per day" },
{ "id": "new_hospitalisations", "label": "New Hospitalisations", "description": "Number of patients who are hospitalised per day" },
{ "id": "new_deaths", "label": "New deaths", "description": "Number of deaths per day" },
{ "id": "new_vaccinations", "label": "New vaccinations", "description": "Number of vaccinations per day" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Could "New deaths" and "New vaccinations" be capitalised to follow the previous few labels? Only an issue if these are going to be displayed in the front end, otherwise don't bother!

Copy link
Contributor

Choose a reason for hiding this comment

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

The house style I'm going for is actually to only capitalize once per title: so it's New Infections and New Hospitalisations that need to change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{ "id": "new_infections", "label": "New Infections", "description": "Number of new infections per day" },
{ "id": "new_hospitalisations", "label": "New Hospitalisations", "description": "Number of patients who are hospitalised per day" },
{ "id": "new_deaths", "label": "New deaths", "description": "Number of deaths per day" },
{ "id": "new_vaccinations", "label": "New vaccinations", "description": "Number of vaccinations per day" }
{ "id": "new_infected", "label": "New infections", "description": "Number of new infections per day" },
{ "id": "new_hospitalised", "label": "New hospitalisations", "description": "Number of patients who are hospitalised per day" },
{ "id": "new_dead", "label": "New deaths", "description": "Number of deaths per day" },
{ "id": "new_vaccinated", "label": "New vaccinations", "description": "Number of vaccinations per day" }

It's neat how the t_s_groups have nouns as their ids while time_series themselves have adjectives as ids -- helps us avoid the ids colliding. So let's regularise on that and have all the time_series adjectives where possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

A suggestion of an edited description to align with the description of the corresponding total (and to be more accurate):

Suggested change
{ "id": "new_infections", "label": "New Infections", "description": "Number of new infections per day" },
{ "id": "new_hospitalisations", "label": "New Hospitalisations", "description": "Number of patients who are hospitalised per day" },
{ "id": "new_deaths", "label": "New deaths", "description": "Number of deaths per day" },
{ "id": "new_vaccinations", "label": "New vaccinations", "description": "Number of vaccinations per day" }
{ "id": "new_infections", "label": "New Infections", "description": "Number of new infections per day" },
{ "id": "new_hospitalisations", "label": "New Hospitalisations", "description": "Number of new patients in need of hospitalisation per day" },
{ "id": "new_deaths", "label": "New deaths", "description": "Number of deaths per day" },
{ "id": "new_vaccinations", "label": "New vaccinations", "description": "Number of vaccinations per day" }

],
"time_series_roles": [
{"id": "total", "label": "Total"},
{"id": "daily", "label": "Daily"}
],
"time_series_groups": [
{
"id": "infections",
"label": "Infections",
"time_series": {
"total": "prevalence",
"daily": "new_infections"
}
},
{
"id": "hospitalisations",
"label": "Hospitalisations",
"time_series": {
"total": "hospitalised",
"daily": "new_hospitalisations"
}
},
{
"id": "deaths",
"label": "Deaths",
"time_series": {
"total": "dead",
"daily": "new_deaths"
}
},
{
"id": "vaccinations",
"label": "Vaccinations",
"time_series": {
"total": "vaccinated",
"daily": "new_vaccinations"
}
}
]
}
}
19 changes: 19 additions & 0 deletions inst/schema/metadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,25 @@
"items": {
"$ref": "#/$defs/displayInfo"
}
},
"time_series_roles": {
"type": "array",
"items": {
"$ref": "#/$defs/displayInfo"
}
},
"time_series_groups": {
"type": "array",
"items": {
"type": "object",
"properties": {
"id": { "type": "string" },
"label": { "type": "string" },
"time_series": { "type": "object" }
Copy link
Contributor

@david-mears-2 david-mears-2 Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is there no way to specify that this object's keys must be 'time_series_role's? If not, what good does it do to define the time_series_roles array?

Copy link
Contributor Author

@EmmaLRussell EmmaLRussell Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so, in the schema. The reason for putting in the time_series_roles array was to include metadata, which the front end might choose to use i.e. label.

},
"additionalProperties": false,
"required": ["id", "label", "time_series"]
}
}
},
"additionalProperties": false,
Expand Down
5 changes: 5 additions & 0 deletions tests/testthat/test-zzz-e2e.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,11 @@ test_that("can run model, get status and results", {
expect_length(results_data$time_series$prevalence, time_series_length)
expect_length(results_data$time_series$hospitalised, time_series_length)
expect_length(results_data$time_series$dead, time_series_length)
expect_length(results_data$time_series$new_infections, time_series_length)
expect_length(results_data$time_series$new_hospitalisations,
time_series_length)
expect_length(results_data$time_series$new_deaths, time_series_length)
expect_length(results_data$time_series$new_vaccinations, time_series_length)

expect_gt(results_data$gdp, 0)

Expand Down
34 changes: 33 additions & 1 deletion tests/testthat/test_model_run.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@ test_that("can run model and return results", {

mockery::stub(model_run, "daedalus::get_data", mock_model_data)

mock_incidence_result <- list(value = c(10, 20))
mock_get_incidence <- mockery::mock(mock_incidence_result,
cycle = TRUE)
mockery::stub(model_run, "daedalus::get_incidence", mock_get_incidence)

mock_new_vaccinations_result <- list(new_vaccinations = c(100, 200))
mockery::stub(model_run, "daedalus::get_new_vaccinations",
mock_new_vaccinations_result)

mock_costs_data <- daedalus_mock_costs()
mock_get_costs <- mockery::mock(mock_costs_data)
mockery::stub(model_run, "daedalus::get_costs", mock_get_costs)
Expand All @@ -39,6 +48,19 @@ test_that("can run model and return results", {
)
)

expect_identical(
mockery::mock_args(mock_get_incidence)[[1]],
list(mock_results, "infections")
)
expect_identical(
mockery::mock_args(mock_get_incidence)[[2]],
list(mock_results, "hospitalisations")
)
expect_identical(
mockery::mock_args(mock_get_incidence)[[3]],
list(mock_results, "deaths")
)

expect_named(res, c(
"parameters",
"costs",
Expand All @@ -50,11 +72,21 @@ test_that("can run model and return results", {
expect_named(res$time_series, c("prevalence",
"hospitalised",
"dead",
"vaccinated"))
"vaccinated",
"new_infections",
"new_hospitalisations",
"new_deaths",
"new_vaccinations"))
expect_identical(res$time_series$prevalence, c(28L, 87L))
expect_identical(res$time_series$hospitalised, c(11L, 31L))
expect_identical(res$time_series$dead, c(15L, 37L))
expect_identical(res$time_series$vaccinated, c(1L, 5L))
expect_identical(res$time_series$new_infections, mock_incidence_result$value)
expect_identical(res$time_series$new_hospitalisations,
mock_incidence_result$value)
expect_identical(res$time_series$new_deaths, mock_incidence_result$value)
expect_identical(res$time_series$new_vaccinations,
mock_new_vaccinations_result$new_vaccinations)
expect_identical(res$parameters, parameters)
expect_nested_mock_costs(res$costs)
expect_identical(res$interventions, list(
Expand Down
Loading