93/PR 3/3 - Propagate performance optimisation throughout checks & validation functions #109
Conversation
…task-ids. Related to #93
Merge branch '98/subset-grid-by-out-type' into 93/3-batch-value-validation # Conflicts: # R/check_tbl_spl_compound_tid.R # R/v3-sample-utils.R # tests/testthat/test-check_tbl_spl_compound_tid.R # tests/testthat/test-check_tbl_spl_non_compound_tid.R
annakrystalli
changed the title
| # } | ||
| # details <- details_bullets_div(details) | ||
| # } | ||
|
|
There was a problem hiding this comment.
This is just obsolete commented out code which I just removed
| ) | ||
| } | ||
| tbl | ||
| } |
There was a problem hiding this comment.
These two function also seem not to be being used anymore so deleted
| @@ -5,18 +5,23 @@ | |||
| #' @inherit check_tbl_colnames params | |||
| #' @inherit check_tbl_col_types return | |||
| #' @export | |||
| check_tbl_values_required <- function(tbl, round_id, file_path, hub_path) { | |||
| check_tbl_values_required <- function(tbl, round_id, file_path, hub_path, | |||
| derived_task_ids = NULL) { | |||
There was a problem hiding this comment.
Unfortunately, this function cannot be batched by output type. Rather it needs to continue to be batched by model task to ensure that if optional task ID values are supplied for a given model task, the correct required output types are also supplied. As such, we need to evaluate model tasks as a whole rather than splitting them unless we complicate this already very complicated function even more. I experimented a bit but decided it wasn't worth the effort at this time. Being able to ignore derived task ids already confers speed ups and I'm hoping I'll be able to gain more through speeding up conc_rows() in the next round of optimisations.
| "i" = "{.arg output_types} must be members of: {.val {round_output_types}}" | ||
| ), | ||
| call = call | ||
| ) | ||
| } | ||
| valid_output_types | ||
| output_types | ||
| } |
There was a problem hiding this comment.
This is the fix requested by @elray1 here: #107 (review)
There was a problem hiding this comment.
Overall, looks good!
- I read through the unit tests fairly carefully. Had a couple of minor questions about whether we might want to check that the
derived_task_idsare truly ignored (not that I doubt they are) - I read the code changes moderately carefully. Most looked clear. I don't know all the places where we might want/need to add the new
derived_task_idsargument, but I trust that you found them. There is a lot of logic in some places, I'm not fresh on this code base, and it's not all documented as thoroughly as I would find helpful. So basically I gave up on trying to understand all the code updates. Asked a question in one place. The fact that the unit tests are passing is reassuring :) - Made a suggestion in the documentation but then saw you had written similar text elsewhere.
I will wait to approve changes till I see what your thoughts are on a couple of these, but also not specifically requesting changes.
| @@ -319,3 +319,31 @@ test_that("validate_submission handles overriding output type id data type corre | |||
| )[["col_types"]] | |||
| ) | |||
| }) | |||
|
|
|||
| test_that("Ignoring derived_task_ids in validate_submission works", { | |||
There was a problem hiding this comment.
similar question to in another test -- to test whether derived_task_ids were actually ignored, it seems like a test would involve setting a derived task id to something invalid and then seeing that an error was not raised?
There was a problem hiding this comment.
I actually think maybe the question is not relevant where I asked it first, but may be relevant here.
There was a problem hiding this comment.
Great points about the tests! I sort of knew from the benchmarks that it was working but admittedly, this wasn't being that explicitly tested throughout (mainly just in the expand_model_out_grid() tests.
So added a bunch of tests throughout following your suggestion of introducing a deliberate error in 23b4655
Co-authored-by: Evan Ray <[email protected]>
This PR completes the first stage of optimising validation performance described in #93
It builds on the functionality introduced to:
and propagates these to relevant
check_*()andvalidate_*()functions to enable output type validation batching as well as the creation of more performant expanded grids during validation.This has lead to significant performance improvements, especially when dealing with complex configs with derived task IDs (see below)
I've also added a section to the documentation to help admins configure their validation workflows to make use of the
derived_taks_idsfeature as at the moment this can only be done manually (but see hubverse-org/schemas#96 for future plans).Benchmarks
So far I have tested with a test model output file and new Respicast config file supplied by @M-7th which includes a derived
target_end_datetask id and have got extremely promising results with these fixes!!View Respicast config
{ "schema_version": "https://raw.githubusercontent.com/hubverse-org/schemas/main/v3.0.1/tasks-schema.json", "rounds": [ { "round_id_from_variable": true, "round_id": "round_id", "model_tasks": [ { "task_ids": { "round_id": { "required": null, "optional": ["2024_2025_1_COVID", "2024_2025_1_FLU"] }, "scenario_id": { "required": null, "optional": ["A", "B", "C", "D", "E", "F"] }, "target": { "required": null, "optional": ["ILI incidence", "ili_plus"] }, "location": { "required": null, "optional": ["AT","BE","BG","CH","CY","CZ","DE","DK","EE","ES","FI","FR","GR","HR","HU","IE","IS","IT","LI","LT","LU","LV","MT","NL","NO","PL","PT","RO","SE","SI","SK","GB-ENG","GB-WLS","GB-NIR","GB-SCT"] }, "pop_group": { "required": null, "optional": ["0-4_vaxYes", "0-4_vaxNo"] }, "target_end_date": { "required": null, "optional": ["2024-08-11","2024-08-18","2024-08-25","2024-09-01","2024-09-08","2024-09-15","2024-09-22","2024-09-29","2024-10-06","2024-10-13","2024-10-20","2024-10-27","2024-11-03","2024-11-10","2024-11-17","2024-11-24","2024-12-01","2024-12-08","2024-12-15","2024-12-22","2024-12-29","2025-01-05","2025-01-12","2025-01-19","2025-01-26","2025-02-02","2025-02-09","2025-02-16","2025-02-23","2025-03-02","2025-03-09","2025-03-16","2025-03-23","2025-03-30","2025-04-06","2025-04-13","2025-04-20","2025-04-27","2025-05-04","2025-05-11","2025-05-18","2025-05-25","2025-06-01"] }, "horizon": { "required": null, "optional": [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43] } }, "output_type": { "sample": { "output_type_id_params": { "is_required": true, "type": "character", "max_length": 6, "min_samples_per_task": 1, "max_samples_per_task": 100, "compound_taskid_set": ["round_id", "scenario_id", "target", "location", "pop_group"] }, "value":{ "type": "double", "minimum": 0 } }, "quantile": { "output_type_id": { "required": null, "optional": [0.01,0.025,0.05,0.1,0.15,0.2,0.25,0.3,0.35,0.4,0.45,0.5,0.55,0.6,0.65,0.7,0.75,0.8,0.85,0.9,0.95,0.975,0.99] }, "value": { "type": "double", "minimum": 0 } } }, "target_metadata": [ { "target_id": "ILI incidence", "target_name": "Weekly incidence for Influenza like illness", "target_units": "cases per 100,000 population", "target_keys": { "target": "ILI incidence" }, "description": "This target represents the count of new ILI cases per 100,000 in the week ending on the date [horizon] weeks after the reference_date", "target_type": "continuous", "is_step_ahead": true, "time_unit": "week" }, { "target_id": "ili_plus", "target_name": "Weekly incidence for Influenza like illness", "target_units": "cases per 100,000 population", "target_keys": { "target": "ili_plus" }, "description": "This target represents the count of new ILI cases per 100,000 in the week ending on the date [horizon] weeks after the reference_date", "target_type": "continuous", "is_step_ahead": true, "time_unit": "week" } ] } ], "submissions_due": {"start":"2024-04-14","end":"2024-09-14"} } ], "output_type_id_datatype": "character" }System Info
Created on 2024-08-16 with reprex v2.1.0