-
Notifications
You must be signed in to change notification settings - Fork 0
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-56 Metadata endpoint #5
Changes from 7 commits
3892fd7
c367bea
57f9c3c
d50e5ff
7da9b95
60e4884
ca8cc41
974e9c2
7fcc989
3dc42b9
bab11eb
974dd4d
c3c6cf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,3 +30,44 @@ | |
) | ||
lapply(versions, function(v) scalar(as.character(v))) | ||
} | ||
|
||
# TODO: specify schema and rerun roxygen2::roxygenize()!! | ||
##' @porcelain GET /metadata => json(metadata) | ||
metadata <- function() { | ||
# TODO: Use relevant model version - from qs if specified, else from latest available metadata file (jidea-62) | ||
Check warning on line 37 in R/api.R GitHub Actions / lint-changed-files
|
||
model_version <- scalar("0.1.0") | ||
# TODO: read in correct metadata version according to model_version (jidea-62) | ||
metadata_file <- sprintf("metadata_%s.json", model_version) | ||
response <- read_json(metadata_file) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was initially confused as to where this function was coming from, but now I see it's redefined in this package; I've added a suggestion to switch to |
||
response$modelVersion <- model_version | ||
|
||
# Helper for the options which don't come from the json | ||
get_option <- function(id, label) { | ||
list(id = scalar(id), label = scalar(label)) | ||
} | ||
|
||
# Set available countries from daedalus package | ||
# TODO: use the right version of daedalus/model | ||
# TODO: get ISO ids from daedalus when available | ||
country_options <- lapply(daedalus::country_names, function(country) { | ||
country_string <- as.character(country) | ||
get_option(country_string, country_string) | ||
}) | ||
country_idx <- match("country", response$parameters$id) | ||
response$parameters$options[[country_idx]] <- country_options | ||
|
||
# TODO: get pathogen information from daedalus, when available (JIDEA-61) | ||
pathogen_options <- list( | ||
get_option("sars-cov-1", "SARS-CoV-1"), | ||
get_option("sars-cov-2-pre-alpha", "SARS-CoV-2 pre-alpha (wildtype)"), | ||
get_option("sars-cov-2-omicron", "SARS-CoV-2 omicron"), | ||
get_option("sars-cov-2-delta", "SARS-CoV-2 delta"), | ||
get_option("influenza-2009", "Influenza 2009 (Swine flu)"), | ||
get_option("influenza-1957", "Influenza 1957"), | ||
get_option("influenza-1918", "Influenza 1918 (Spanish flu)") | ||
) | ||
pathogen_idx <- match("pathogen", response$parameters$id) | ||
response$parameters$options[[pathogen_idx]] <- pathogen_options | ||
|
||
response | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# Allowed parameter ids which we can pass to the model | ||
expected_parameters = c( | ||
"country", | ||
"pathogen", | ||
"response", | ||
"vaccine" | ||
) |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,3 +5,17 @@ scalar <- function(x) { | |
package_version_string <- function(name) { | ||
as.character(utils::packageVersion(name)) | ||
} | ||
|
||
system_file <- function(...) { | ||
tryCatch({ | ||
system.file(..., mustWork = TRUE, package = "daedalus.api") | ||
}, error = function(e) { | ||
stop(sprintf("Failed to locate file from args\n%s", | ||
paste(list(...), collapse = " "))) | ||
}) | ||
} | ||
Comment on lines
+10
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handy method stolen from hintr!
Comment on lines
+10
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see why this function has been included, although it's over-writing a well known base function; the informative error is useful. Could we get a small comment explaining that? |
||
|
||
read_json <- function(filename) { | ||
json <- jsonlite::fromJSON(system_file("json", filename)) | ||
json | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this be replaced by the {jsonlite} function of the same name, with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah good point. How about if I change the name to something not shared with jsonlite to make it clear this is a special for the API? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
{ | ||
"modelVersion": null, | ||
"parameters": [ | ||
{ | ||
"id": "country", | ||
"label": "Country", | ||
"parameterType": "globeSelect", | ||
"defaultOption": null, | ||
"ordered": false, | ||
"options": null | ||
}, | ||
{ | ||
"id": "pathogen", | ||
"label": "Disease", | ||
"parameterType": "select", | ||
"defaultOption": null, | ||
"ordered": false, | ||
"options": null | ||
}, | ||
{ | ||
"id": "response", | ||
"label": "Response", | ||
"parameterType": "select", | ||
"defaultOption": "no_closure", | ||
"ordered": true, | ||
"options": [ | ||
{ "id": "no_closure", "label": "No closures" }, | ||
{ "id": "school_closure", "label": "School closures" }, | ||
{ "id": "business_closure", "label": "Business closures" }, | ||
{ "id": "elimination", "label": "Elimination" } | ||
] | ||
}, | ||
{ | ||
"id": "vaccine", | ||
"label": "Advance vaccine investment", | ||
"parameterType": "select", | ||
"defaultOption": "none", | ||
"ordered": true, | ||
"options": [ | ||
{ "id": "none", "label": "None" }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more of a question than a comment, but it's possible in the future we'll need to annotate each such option with a description to explain what precisely is meant by e.g. 'Medium'. I presume it's OK to cross that bridge when we get to it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we can add a "description" field if required. |
||
{ "id": "low", "label": "Low" }, | ||
{ "id": "medium", "label": "Medium" }, | ||
{ "id": "high", "label": "High" } | ||
] | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
{ | ||
"$schema": "http://json-schema.org/draft-04/schema#", | ||
"type": "object", | ||
"properties": { | ||
"modelVersion": { | ||
"type": "string", | ||
"pattern": "^[0-9]+(\\.[0-9]+)*$" | ||
}, | ||
"parameters":{ | ||
"type": "array", | ||
"items": { | ||
"type": "object", | ||
"properties": { | ||
"id": { "type": "string" }, | ||
"label": { "type": "string" }, | ||
"parameterType": { | ||
"type": "string", | ||
"enum": ["select", "globeSelect"] | ||
}, | ||
"defaultOption": { | ||
"type": ["string", "null"] | ||
}, | ||
"ordered": { "type": "boolean" }, | ||
"options": { | ||
"type": "array", | ||
"items": { | ||
"type": "object", | ||
"properties":{ | ||
"id": { "type": "string" }, | ||
"label": { "type": "string" } | ||
}, | ||
"additionalProperties": false, | ||
"required": ["id", "label"] | ||
} | ||
} | ||
}, | ||
"additionalProperties": false, | ||
"required": ["id", "label", "parameterType", "defaultOption", "ordered", "options"] | ||
} | ||
} | ||
}, | ||
"additionalProperties": false, | ||
"required": ["modelVersion", "parameters"] | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -25,3 +25,23 @@ test_that("Can construct the api", { | |||||||||
expect_length(logs, 2L) | ||||||||||
expect_identical(logs[[1L]]$logger, "daedalus.api") | ||||||||||
}) | ||||||||||
|
||||||||||
test_that("Can get metadata", { | ||||||||||
endpoint <- daedalus_api_endpoint("GET", "/metadata") | ||||||||||
res <- endpoint$run() | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could I suggest a no conditions expectation here:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, and I've put it on the root call too.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha, that upsets the linter though: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yes, a classic. The other linter complaints can usually be fixed with a |
||||||||||
expect_true(res$validated) | ||||||||||
|
||||||||||
lapply(res$data$parameters$id, function(id){ | ||||||||||
expect_true(id %in% expected_parameters) | ||||||||||
}) | ||||||||||
|
||||||||||
# expect country ids to match those from daedalus | ||||||||||
country_idx <- match("country", res$data$parameters$id) | ||||||||||
country_options <- res$data$parameters$options[[country_idx]] | ||||||||||
daedalus_countries = daedalus::country_names | ||||||||||
expect_equal(length(country_options), length(daedalus_countries)) | ||||||||||
lapply(seq_along(country_options), function(idx) { | ||||||||||
expect_equal(country_options[[idx]]$id, scalar(daedalus_countries[[idx]])) | ||||||||||
expect_equal(country_options[[idx]]$label, scalar(daedalus_countries[[idx]])) | ||||||||||
}) | ||||||||||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this pull from the package version for now? The model structure and data versions are all bundled together in the package version at the moment anyway.