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.

Sign up for GitHub

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

Closes 2661 - ignore_seconds_flag now defaults to TRUE. #2670

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

Closes 2661 - ignore_seconds_flag now defaults to TRUE. #2670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
a201383
2661 - `ignore_seconds_flag` now defaults to TRUE. Placeholder only.…
jimrothstein Feb 26, 2025
f36a84f
fix: #2661 ignore_seconds_flag set to TRUE removed :xy from examples
bms63 Mar 2, 2025
1fe9458
2661 cleanup
jimrothstein Mar 2, 2025
d5ea3c1
2661 remove debug (browser())
jimrothstein Mar 2, 2025
8a06e7a
Merge branch '2661-general-issue-ignore_seconds_flag-true-is-set-as-d…
jimrothstein Mar 2, 2025
fe6f414
2661 -
jimrothstein Mar 2, 2025
e35a098
2661 - Test #18 now passes (removed and adjusted rows in expected_out…
jimrothstein Mar 2, 2025
4d04611
# 2661 - Test # 19 now PASSES
jimrothstein Mar 2, 2025
cd17a4f
2661 Test # 20 - PASSES, with 1 removal in actual_output
jimrothstein Mar 3, 2025
fa6beef
2661 update test-derive-dtm.R (not finished)
jimrothstein Mar 3, 2025
13bbd40
2661 - TEST # 31 PASSES (b/c changed error = FALSE)
jimrothstein Mar 3, 2025
9723e34
2661 Drop duplicate line in examples (mhdt, 2019-07-18T15:25)
jimrothstein Mar 3, 2025
1f32d83
2661 ignore_seconds_flag in compute_tmf is NOW TRUE, as default
jimrothstein Mar 3, 2025
d5ba89c
2661 - test-derive_vars_dtm.R, Test #14,
jimrothstein Mar 3, 2025
90f9ff1
2661 Add NOTE default value of `ignore_seconds_flag` is now TRUE
jimrothstein Mar 3, 2025
2e0d287
2661 - a few changes per Bundfssr - more to come, but ask him to take…
jimrothstein Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@

## Breaking Changes

-
- The following function arguments are entering the next phase of the [deprecation process](https://pharmaverse.github.io/admiraldev/articles/programming_strategy.html#deprecation): (#2487) (#2595)

**Phase 1 (message)**

- (placeholder - waiting for BMS63) `ignore_seconds_flag` set to `TRUE` in functions `derive_vars_dtm` and `compute_tmf`

**Phase 2 (warning)**

**Phase 3 (error)**
Expand Down
31 changes: 28 additions & 3 deletions R/derive_vars_dtm.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
#' The date and time can be imputed (see `date_imputation`/`time_imputation` arguments)
#' and the date/time imputation flag (`'--DTF'`, `'--TMF'`) can be added.
#'
#' NOTE: The default value for `ignore_seconds_flag` is `TRUE` (as of v 1.4?).
#' An error will be thrown if `--DTC` contains seconds. SEE examples.
#'
#' In `{admiral}` we don't allow users to pick any single part of the date/time to
#' impute, we only enable to impute up to a highest level, i.e. you couldn't
#' choose to say impute months, but not days.
Expand Down Expand Up @@ -58,7 +61,6 @@
#'
#' mhdt <- tribble(
#' ~MHSTDTC,
#' "2019-07-18T15:25:40",
#' "2019-07-18T15:25",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The first and the second record are the same. One of them should be removed.

Further updates of the examples are required as the first and third example are the same.

  • The third example should be updated such that it shows the behaviour when ignore_seconds_flag = FALSE is used.
  • An example should be added to show that an error is issued if seconds are present in the DTC variable and ignore_seconds_flag is not set to FALSE. This should also be mentioned in the description of the ignore_seconds_flag argument.

Copy link
Collaborator Author

@jimrothstein jimrothstein Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@bundfussr
Working on the changes ... thanks

An example should be added to show that an error is issued if seconds are present in the DTC variable and >ignore_seconds_flag is not set to FALSE.

Meaning: "2019-07-18T15:25:40" will throw error when ignore_seconds_flag is DEFAULT (ie TRUE)?

  • By hand, I will work out a chart of what happens, given --DTF and various parameters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Meaning: "2019-07-18T15:25:40" will throw error when ignore_seconds_flag is DEFAULT (ie TRUE)?

Yes

#' "2019-07-18",
#' "2019-02",
Expand All @@ -74,6 +76,29 @@
#' highest_imputation = "M"
#' )
#'
#' # Repeat, but with `ignore_seconds_flag = FALSE`
#' derive_vars_dtm(
#' mhdt,
#' new_vars_prefix = "AST",
#' dtc = MHSTDTC,
#' highest_imputation = "M",
#' ignore_seconds_flag = FALSE
#' )
#'
#' # Error is thrown when data includes seconds, but `ignore_seconds_flag`
#' # is default value (TRUE)
#' mhdt <- tribble(
#' ~MHSTDTC,
#' "2019-07-18T15:25:40",
#' )
#' try(
#' derive_vars_dtm(
#' mhdt,
#' new_vars_prefix = "AST",
#' dtc = MHSTDTC,
#' highest_imputation = "M"
#' ))
#'
#' # Impute AE end date to the last date and ensure that the imputed date is not
#' # after the death or data cut off date
#' adae <- tribble(
Expand Down Expand Up @@ -135,7 +160,7 @@ derive_vars_dtm <- function(dataset, # nolint: cyclocomp_linter
min_dates = NULL,
max_dates = NULL,
preserve = FALSE,
ignore_seconds_flag = FALSE) {
ignore_seconds_flag = TRUE) {
# check and quote arguments
assert_character_scalar(new_vars_prefix)
assert_vars(max_dates, optional = TRUE)
Expand Down Expand Up @@ -711,7 +736,7 @@ restrict_imputed_dtc_dtm <- function(dtc,
#'
compute_tmf <- function(dtc,
dtm,
ignore_seconds_flag = FALSE) {
ignore_seconds_flag = TRUE) {
assert_date_vector(dtm)
assert_character_vector(dtc)
assert_logical_scalar(ignore_seconds_flag)
Expand Down
4 changes: 2 additions & 2 deletions man/derive_vars_dtm.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 12 additions & 5 deletions tests/testthat/_snaps/derive_vars_dtm.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
# derive_vars_dtm Test 22: No re-derivation is done if --DTF variable already exists

Code
actual_output <- derive_vars_dtm(mutate(input, ASTDTF = c(NA, NA, NA, NA, "D",
"MD", "M")), new_vars_prefix = "AST", dtc = XXSTDTC, highest_imputation = "M",
actual_output <- derive_vars_dtm(mutate(input, ASTDTF = c(NA, NA, NA, "D", "MD",
"M")), new_vars_prefix = "AST", dtc = XXSTDTC, highest_imputation = "M",
date_imputation = "first")
Message
The `ASTDTF` variable is already present in the input dataset and will not be re-derived.
Expand Down Expand Up @@ -64,7 +64,14 @@
Code
derive_vars_dtm(input, new_vars_prefix = "AST", dtc = XXSTDTC,
ignore_seconds_flag = TRUE)
Condition
Error in `derive_vars_dtm()`:
! Seconds detected in data while `ignore_seconds_flag` is invoked
Output
# A tibble: 6 x 3
XXSTDTC ASTDTM ASTTMF
<chr> <dttm> <chr>
1 2019-07-18T15:25 2019-07-18 15:25:00 <NA>
2 2019-07-18T15 2019-07-18 15:00:00 M
3 2019-07-18 2019-07-18 00:00:00 H
4 2019-02 NA <NA>
5 2019 NA <NA>
6 2019---07 NA <NA>

31 changes: 17 additions & 14 deletions tests/testthat/test-derive_vars_dtm.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,8 @@ test_that("compute_tmf Test 14: compute TMF", {
expect_equal(
compute_tmf(
dtc = input_dtc,
dtm = input_dtm
dtm = input_dtm,
ignore_seconds_flag = FALSE
),
expected_output
)
Expand Down Expand Up @@ -383,7 +384,6 @@ test_that("compute_tmf Test 16: ignore_seconds_flag = TRUE", {

input <- tibble::tribble(
~XXSTDTC,
"2019-07-18T15:25:40",
"2019-07-18T15:25",
"2019-07-18T15",
"2019-07-18",
Expand All @@ -396,8 +396,7 @@ input <- tibble::tribble(
test_that("derive_vars_dtm Test 17: default behavior", {
expected_output <- tibble::tribble(
~XXSTDTC, ~ASTDTM, ~ASTTMF,
"2019-07-18T15:25:40", ymd_hms("2019-07-18T15:25:40"), NA_character_,
"2019-07-18T15:25", ymd_hms("2019-07-18T15:25:00"), "S",
"2019-07-18T15:25", ymd_hms("2019-07-18T15:25:00"), NA,
"2019-07-18T15", ymd_hms("2019-07-18T15:00:00"), "M",
"2019-07-18", ymd_hms("2019-07-18T00:00:00"), "H",
"2019-02", ymd_hms(NA), NA_character_,
Expand All @@ -422,8 +421,7 @@ test_that("derive_vars_dtm Test 17: default behavior", {
test_that("derive_vars_dtm Test 18: date imputed to first, auto DTF/TMF", {
expected_output <- tibble::tribble(
~XXSTDTC, ~ASTDTM, ~ASTDTF, ~ASTTMF,
"2019-07-18T15:25:40", ymd_hms("2019-07-18T15:25:40"), NA_character_, NA_character_,
"2019-07-18T15:25", ymd_hms("2019-07-18T15:25:00"), NA_character_, "S",
"2019-07-18T15:25", ymd_hms("2019-07-18T15:25:00"), NA_character_, NA,
"2019-07-18T15", ymd_hms("2019-07-18T15:00:00"), NA_character_, "M",
"2019-07-18", ymd_hms("2019-07-18T00:00:00"), NA_character_, "H",
"2019-02", ymd_hms("2019-02-01T00:00:00"), "D", "H",
Expand All @@ -450,7 +448,6 @@ test_that("derive_vars_dtm Test 18: date imputed to first, auto DTF/TMF", {
test_that("derive_vars_dtm Test 19: date and time imputed to last, no DTF/TMF", {
expected_output <- tibble::tribble(
~XXSTDTC, ~AENDTM,
"2019-07-18T15:25:40", ymd_hms("2019-07-18T15:25:40"),
"2019-07-18T15:25", ymd_hms("2019-07-18T15:25:59"),
"2019-07-18T15", ymd_hms("2019-07-18T15:59:59"),
"2019-07-18", ymd_hms("2019-07-18T23:59:59"),
Expand Down Expand Up @@ -480,7 +477,6 @@ test_that("derive_vars_dtm Test 19: date and time imputed to last, no DTF/TMF",
test_that("derive_vars_dtm Test 20: date and time imputed to last, DTF only", {
expected_output <- tibble::tribble(
~XXSTDTC, ~AENDTM, ~AENDTF,
"2019-07-18T15:25:40", ymd_hms("2019-07-18T15:25:40"), NA_character_,
"2019-07-18T15:25", ymd_hms("2019-07-18T15:25:59"), NA_character_,
"2019-07-18T15", ymd_hms("2019-07-18T15:59:59"), NA_character_,
"2019-07-18", ymd_hms("2019-07-18T23:59:59"), NA_character_,
Expand Down Expand Up @@ -510,8 +506,7 @@ test_that("derive_vars_dtm Test 20: date and time imputed to last, DTF only", {
test_that("derive_vars_dtm Test 21: date imputed to MID, time to first, TMF only", {
expected_output <- tibble::tribble(
~XXSTDTC, ~ASTDTM, ~ASTTMF,
"2019-07-18T15:25:40", ymd_hms("2019-07-18T15:25:40"), NA_character_,
"2019-07-18T15:25", ymd_hms("2019-07-18T15:25:00"), "S",
"2019-07-18T15:25", ymd_hms("2019-07-18T15:25:00"), NA,
"2019-07-18T15", ymd_hms("2019-07-18T15:00:00"), "M",
"2019-07-18", ymd_hms("2019-07-18T00:00:00"), "H",
"2019-02", ymd_hms("2019-02-15T00:00:00"), "H",
Expand Down Expand Up @@ -540,8 +535,7 @@ test_that("derive_vars_dtm Test 21: date imputed to MID, time to first, TMF only
test_that("derive_vars_dtm Test 22: No re-derivation is done if --DTF variable already exists", {
expected_output <- tibble::tribble(
~XXSTDTC, ~ASTDTM, ~ASTDTF, ~ASTTMF,
"2019-07-18T15:25:40", ymd_hms("2019-07-18T15:25:40"), NA_character_, NA_character_,
"2019-07-18T15:25", ymd_hms("2019-07-18T15:25:00"), NA_character_, "S",
"2019-07-18T15:25", ymd_hms("2019-07-18T15:25:00"), NA_character_, NA,
"2019-07-18T15", ymd_hms("2019-07-18T15:00:00"), NA_character_, "M",
"2019-07-18", ymd_hms("2019-07-18T00:00:00"), NA_character_, "H",
"2019-02", ymd_hms("2019-02-01T00:00:00"), "D", "H",
Expand All @@ -552,7 +546,7 @@ test_that("derive_vars_dtm Test 22: No re-derivation is done if --DTF variable a

expect_snapshot(
actual_output <- derive_vars_dtm(
mutate(input, ASTDTF = c(NA, NA, NA, NA, "D", "MD", "M")),
mutate(input, ASTDTF = c(NA, NA, NA, "D", "MD", "M")),
new_vars_prefix = "AST",
dtc = XXSTDTC,
highest_imputation = "M",
Expand Down Expand Up @@ -781,13 +775,22 @@ test_that("derive_vars_dtm Test 30: Supplying both min/max dates for highest_imp
})

## Test 31: catch ignore_seconds_flag error ----

input <- tibble::tribble(
~XXSTDTC,
"2019-07-18T15:25:40",
"2019-07-18T15",
"2019-07-18",
"2019-02",
"2019",
"2019---07"
)
test_that("derive_vars_dtm Test 31: catch ignore_seconds_flag error", {
expect_snapshot(
derive_vars_dtm(
input,
new_vars_prefix = "AST",
dtc = XXSTDTC,
ignore_seconds_flag = TRUE
),
error = TRUE
)
Expand Down
13 changes: 8 additions & 5 deletions vignettes/imputation.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ imputation flags are valid for both the datetime and the date variable.
```{r}
ae <- tribble(
~AESTDTC,
"2019-08-09T12:34:56",
"2019-08-09T12:34",
"2019-04-12",
"2010-09",
NA_character_
Expand Down Expand Up @@ -363,7 +363,7 @@ no date is imputed the date imputation flag is not created.
```{r}
ae <- tribble(
~AESTDTC,
"2019-08-09T12:34:56",
"2019-08-09T12:34",
"2019-04-12",
"2010-09",
NA_character_
Expand Down Expand Up @@ -406,7 +406,8 @@ ae <- tribble(
highest_imputation = "M",
date_imputation = "first",
time_imputation = "first",
min_dates = exprs(TRTSDTM)
min_dates = exprs(TRTSDTM),
ignore_seconds_flag = FALSE
)
```
```{r, echo=FALSE}
Expand Down Expand Up @@ -438,7 +439,8 @@ ae <- tribble(
highest_imputation = "M",
date_imputation = "last",
time_imputation = "last",
max_dates = exprs(DTHDT, DCUTDT)
max_dates = exprs(DTHDT, DCUTDT),
ignore_seconds_flag = FALSE
)
```
```{r, echo=FALSE}
Expand Down Expand Up @@ -488,7 +490,8 @@ vs <- tribble(
derivation = derive_vars_dtm,
args = params(
dtc = VSDTC,
new_vars_prefix = "A"
new_vars_prefix = "A",
ignore_seconds_flag = FALSE
),
derivation_slice(
filter = VSTPT == "PRE-DOSE",
Expand Down
3 changes: 2 additions & 1 deletion vignettes/pk_adnca.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ pc_dates <- pc %>%
derive_vars_dtm(
new_vars_prefix = "A",
dtc = PCDTC,
time_imputation = "00:00:00"
time_imputation = "00:00:00",
ignore_seconds_flag = FALSE
) %>%
# Derive dates and times from date/times
derive_vars_dtm_to_dt(exprs(ADTM)) %>%
Expand Down
Loading