-
Notifications
You must be signed in to change notification settings - Fork 3
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
Encode point estimate output type IDs with null. #111
Encode point estimate output type IDs with null. #111
Conversation
/diff |
Here are your diffs for this pull request
|
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.
I made a couple of minor wording suggestions, but leave it up to you what you think is clearer. Approving.
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.
I think setting required: null
for the point estimates makes the most sense.
I also wanted to also provide context for what we can expect for adapting
hubValidations
to work with this new format, becuase setting
required: null
will make point estimates invisible to
expand_model_out_grid
as demonstrated below.
Setup: creating three configurations:
v3
the v3 config where mean hasrequired: ["NA"]
array_null
the v3 config, but the mean hasrequired: [null]
solo_null
the v3 config, but the mean hasrequired: null
hub_con <- hubData::connect_hub(
system.file("testhubs/simple", package = "hubUtils")
)
hp <- attributes(hub_con)$hub_path
# Get the tasks file which has a `mean` output type
tasks <- fs::path(hp, "hub-config", "tasks.json")
# copy the file to a new temp file
cfg <- readLines(tasks)
# change `required:` to a bare `null`
nullcfg <- sub('["NA"]', 'null', cfg, fixed = TRUE)
# change `required:` to a `[null]` array
null_array_cfg <- sub('["NA"]', '[null]', cfg, fixed = TRUE)
# convert to arrays
v3 <- jsonlite::fromJSON(cfg, simplifyDataFrame = FALSE)
array_null <- jsonlite::fromJSON(null_array_cfg, simplifyDataFrame = FALSE)
solo_null <- jsonlite::fromJSON(nullcfg, simplifyDataFrame = FALSE)
null array matches our current infrastructure, but bare null is different
identical(v3, array_null)
#> [1] TRUE
identical(v3, solo_null)
#> [1] FALSE
The implication for this is that expand_model_out_grid()
no longer detects
point estimates. The omission happens in expand_model_out_grid.R#214
with purrr::compact()
throwing away list(mean = NULL)
. I think we can
address this by adding a v4 catch at that point that protects point estimates.
library("hubValidations")
head(expand_model_out_grid(v3, "2022-10-01"))
#> # A tibble: 6 × 6
#> origin_date target horizon location output_type output_type_id
#> <date> <chr> <int> <chr> <chr> <dbl>
#> 1 2022-10-01 wk inc flu hosp 1 US mean NA
#> 2 2022-10-01 wk inc flu hosp 2 US mean NA
#> 3 2022-10-01 wk inc flu hosp 3 US mean NA
#> 4 2022-10-01 wk inc flu hosp 4 US mean NA
#> 5 2022-10-01 wk inc flu hosp 1 01 mean NA
#> 6 2022-10-01 wk inc flu hosp 2 01 mean NA
head(expand_model_out_grid(array_null, "2022-10-01"))
#> # A tibble: 6 × 6
#> origin_date target horizon location output_type output_type_id
#> <date> <chr> <int> <chr> <chr> <dbl>
#> 1 2022-10-01 wk inc flu hosp 1 US mean NA
#> 2 2022-10-01 wk inc flu hosp 2 US mean NA
#> 3 2022-10-01 wk inc flu hosp 3 US mean NA
#> 4 2022-10-01 wk inc flu hosp 4 US mean NA
#> 5 2022-10-01 wk inc flu hosp 1 01 mean NA
#> 6 2022-10-01 wk inc flu hosp 2 01 mean NA
head(expand_model_out_grid(solo_null, "2022-10-01"))
#> # A tibble: 6 × 6
#> origin_date target horizon location output_type output_type_id
#> <date> <chr> <int> <chr> <chr> <dbl>
#> 1 2022-10-01 wk inc flu hosp 1 US quantile 0.01
#> 2 2022-10-01 wk inc flu hosp 2 US quantile 0.01
#> 3 2022-10-01 wk inc flu hosp 3 US quantile 0.01
#> 4 2022-10-01 wk inc flu hosp 4 US quantile 0.01
#> 5 2022-10-01 wk inc flu hosp 1 01 quantile 0.01
#> 6 2022-10-01 wk inc flu hosp 2 01 quantile 0.01
Created on 2024-10-25 with reprex v2.1.1
Thanks @zkamvar . None of the proposed schema changes work yet because the functionality to support it hasn't been developed yet. The usual development process is to queue up the new version in |
Co-authored-by: Evan Ray <[email protected]>
Co-authored-by: Evan Ray <[email protected]>
Resolves #109