Plotthedots generates an error if multiple events are submitted on a single day

[francisbarton]

If more than two rare events occur on a single date, NHSRplotthedots::ptd_spc(), which is called by SPCreporter:::make_spc_data(), gives an error.

(Error is generated specifically by ptd_validate_spc_options)

Reprex:

library(lubridate, warn.conflicts = FALSE)

test_data <- list(
  month = tibble::tribble(
    ~ref, ~measure_name, ~comment, ~`2023-12-01`,
    "100", "More data", "Test", 1
  ),
  events = tibble::tribble(
    ~ref, ~measure_name, ~comment, ~event_date_or_datetime,
    "1", "Test data", "Reprex for duplicate date", as_date("2023-12-18"),
    "1", "Test data", "Reprex for duplicate date", as_date("2023-12-18"),
    "1", "Test data", "Reprex for duplicate date", as_date("2023-12-18")
  )
)

test_rc <- tibble::tribble(
  ~ref, ~measure_name, ~domain, ~spc_chart_type, ~aggregation, ~report_comment,
  "1", "Test data", NA_character_, "t", "none", NA_character_
)

test_mc <- tibble::tribble(
  ~ref, ~measure_name, ~unit, ~data_source, ~data_owner, ~accountable_person, ~improvement_direction, ~target, ~target_set_by, ~data_quality, ~rebase_dates, ~rebase_comment,
  "1", "Test data", "integer", "My brain", "Me", "nobody", "decrease", 0, "Me", "GGGG", "", ""
)

data_cutoff_dttm <- as_datetime("2023-12-31 23:59:59")
test_data_bundle <- SPCreporter::spcr_make_data_bundle(
  test_data, test_rc, test_mc, data_cutoff_dttm
)
#> ℹ check_for_optional_columns: Optional column 'allowable_days_lag' is missing.
#> Adding it.

test_data_bundle |>
  dplyr::select(all_of(c(
    "target",
    "rebase_dates",
    "improvement_direction",
    "measure_data"
  ))) |>
  purrr::pmap(SPCreporter:::make_spc_data, .progress = "SPC data")
#> Error in `purrr::pmap()`:
#> ℹ In index: 1.
#> Caused by error:
#> ! duplicate rows found in 'date'
#> Backtrace:
#>      ▆
#>   1. ├─purrr::pmap(...)
#>   2. │ └─purrr:::pmap_("list", .l, .f, ..., .progress = .progress)
#>   3. │   ├─purrr:::with_indexed_errors(...)
#>   4. │   │ └─base::withCallingHandlers(...)
#>   5. │   ├─purrr:::call_with_cleanup(...)
#>   6. │   └─SPCreporter (local) .f(...)
#>   7. │     ├─NHSRplotthedots::ptd_spc(...) at SPCreporter/R/spcr_make_report.R:205:3
#>   8. │     └─NHSRplotthedots:::ptd_spc.data.frame(...) at NHSRplotthedots/R/ptd_spc.R:97:3
#>   9. │       └─NHSRplotthedots:::ptd_validate_spc_options(options, .data) at NHSRplotthedots/R/ptd_spc.R:135:3
#>  10. │         └─assertthat::assert_that(...) at NHSRplotthedots/R/ptd_validate_spc_options.R:25:3
#>  11. │           └─base::stop(assertError(attr(res, "msg")))
#>  12. └─purrr (local) `<fn>`(`<assrtErr>`)
#>  13.   └─cli::cli_abort(...)
#>  14.     └─rlang::abort(...)

[ThomUK]

Thanks - we need to correct this with the help of some unit tests to document and exercise the edge-cases in the test suite. Hopeful that using date-times not just dates will solve, but I need to investigate.

[ThomUK]

Human Input Needed

The current make_spc_data() function already contains a dplyr::slice_max(value, n = 1, with_ties = FALSE, by = "date") step that deduplicates rows with the same date before passing data to ptd_spc(). This appears to address the error described in the reprex.

Please verify whether the current code resolves this issue. If confirmed, this issue can be closed. If there are remaining edge cases (e.g. the semantics of keeping 'max value' vs. 'first event' for T-charts), please clarify the expected behaviour.

[ThomUK]

The current code gives the following result, which is not acceptable:

> library(lubridate, warn.conflicts = FALSE)
> test_data <- list(
+   month = tibble::tribble(
+     ~ref, ~measure_name, ~comment, ~`2023-12-01`,
+     "100", "More data", "Test", 1
+   ),
+   events = tibble::tribble(
+     ~ref, ~measure_name, ~comment, ~event_date_or_datetime,
+     "1", "Test data", "Reprex for duplicate date", as_date("2023-12-18"),
+     "1", "Test data", "Reprex for duplicate date", as_date("2023-12-18"),
+     "1", "Test data", "Reprex for duplicate date", as_date("2023-12-18")
+   )
+ )
> test_rc <- tibble::tribble(
+   ~ref, ~measure_name, ~domain, ~spc_chart_type, ~aggregation, ~report_comment,
+   "1", "Test data", NA_character_, "t", "none", NA_character_
+ )
> test_mc <- tibble::tribble(
+   ~ref, ~measure_name, ~unit, ~data_source, ~data_owner, ~accountable_person, ~improvement_direction, ~target, ~target_set_by, ~data_quality, ~rebase_dates, ~rebase_comment,
+   "1", "Test data", "integer", "My brain", "Me", "nobody", "decrease", 0, "Me", "GGGG", "", ""
+ )
> data_cutoff_dttm <- as_datetime("2023-12-31 23:59:59")
> test_data_bundle <- SPCreporter::spcr_make_data_bundle(
+   test_data, test_rc, test_mc, data_cutoff_dttm
+ )
ℹ check_for_optional_columns: Optional column 'allowable_days_lag' is missing. Adding it.
Error: check_for_duplicate_dates: Duplicate dates found in the data. This can happen when data is accidentally supplied from two sources. First duplicate: ref '1', aggregation 'none', date '2023-12-18'.

Duplicate dates are valid within the event data, but not within the aggregated data. A duplicate date within event data means the time between them was 0. A duplicate date within aggregated data indicates malformed input.

[ThomUK]

Implementation Plan

Fix check_for_duplicate_dates() to only validate aggregated data, not event/t-chart data where duplicate dates are valid (multiple events on the same day = 0 days between events).

Approach:

• Move the check_for_duplicate_dates() call in spcr_make_data_bundle.R to run on the aggregated-data-only long frame (before binding event rows), so that legitimate duplicate dates in t-chart event data do not trigger the check
• Update any relevant tests to cover the case of multiple events on the same day passing validation

Tests:

• check_for_duplicate_dates() still errors on duplicate dates in aggregated data
• spcr_make_data_bundle() succeeds when two or more events share the same date (aggregation = 'none')