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

jidea-110 New time series #30

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

EmmaLRussell
Copy link
Contributor

This branch adds time series for new (daily) values for infections, hospitalisations, deaths and vaccinations, using daedalus's get_incidence and get_new_vaccinations methods.

It also adds metadata for the new time series, and for groupings of time series. This might overkill, but I wanted the API to provide as much information as possible so that the front end could be as generic as possible - but also has the option to pick and choose which metadata it wants to use. The idea here is that related time_series are grouped together into time_series_groups, and each time series in a group takes a particular "role" ie total or daily. The role the time series takes in a group is its key in time_series_groups.times_series, and the top level time_series_roles provides metadata for the roles (currently just label but could add description too). Could also add descriptions for the time series groups, just let me know.

@EmmaLRussell EmmaLRussell marked this pull request as ready for review October 21, 2024 12:58
"properties": {
"id": { "type": "string" },
"label": { "type": "string" },
"time_series": { "type": "object" }
Copy link
Contributor

@david-mears-2 david-mears-2 Oct 21, 2024

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

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.

Copy link
Contributor

@pratikunterwegs pratikunterwegs left a comment

Choose a reason for hiding this comment

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

Thanks @EmmaLRussell - just a couple of comments from me, but otherwise feel free to merge this once David okays it.

R/model_run.R Outdated
Comment on lines 44 to 52
# 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

Comment on lines 113 to 114
{ "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.

Comment on lines 111 to 114
{ "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.

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.

Comment on lines 111 to 114
{ "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.

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" }

Copy link
Contributor

@david-mears-2 david-mears-2 left a comment

Choose a reason for hiding this comment

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

Thanks for getting this out of the door so quickly. See comments.

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.

3 participants