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

253 feature request deprecatesupersede derive vars crit #254

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

253 feature request deprecatesupersede derive vars crit #254

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ec72fdd
Substituted derive_vars_crit with new admiral function: derive_vars_c…
federicobaratin Dec 12, 2024
19f3c2a
Updated derive_vars_crit_flag function documentation.
federicobaratin Dec 12, 2024
08353a7
Adehering to deprecation strategy.
federicobaratin Dec 12, 2024
ea1ae6c
run roxygen step.
federicobaratin Dec 13, 2024
6c132ba
added CRIT1FLN in ADIS vignette.
federicobaratin Dec 13, 2024
6b0196e
Corrected typo.
federicobaratin Dec 13, 2024
85e7128
Replace old fun body with derive_vars_crit_flag and update test cases
ahasoplakus Jan 13, 2025
6c8f043
fix vignettes and ignore links
ahasoplakus Jan 13, 2025
158a507
appease lychee
ahasoplakus Jan 13, 2025
15e74e7
fix template
ahasoplakus Jan 13, 2025
9e97389
Update news
ahasoplakus Jan 13, 2025
5a9571c
Increment version number to 0.4.0
ahasoplakus Jan 15, 2025
e5d41e3
add deprecation test
ahasoplakus Jan 15, 2025
5623e15
Updated function as per deprecation strategy, Phase 1.
federicobaratin Jan 15, 2025
09c2f16
Rre-run function
federicobaratin Jan 16, 2025
6b660f6
Revert commit
federicobaratin Jan 16, 2025
5e43e5e
Added deprecation message and fixed tests.
federicobaratin Jan 16, 2025
7a62636
Put depracte_warn().
federicobaratin Jan 17, 2025
76d8802
Restored old function
federicobaratin Jan 17, 2025
69f98dd
run roxygen.
federicobaratin Jan 17, 2025
f5e454e
Put rlang
federicobaratin Jan 17, 2025
fc814f2
Put input as workaround
federicobaratin Jan 17, 2025
46ef0db
Removed recall to the other function.
federicobaratin Jan 19, 2025
8ec9bf1
Added test for deprecation.
federicobaratin Jan 19, 2025
72f8481
Test 4 for deprecation is not working.
federicobaratin Jan 19, 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
145 changes: 6 additions & 139 deletions R/derive_vars_crit.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,145 +2,12 @@
#'
#'
#' @description
#' Derive analysis criterion evaluation result variable, paired with character
Copy link

Choose a reason for hiding this comment

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

why delete this section? i think ok to keep, just need to add the badge.

#' and numeric flags.
#' This function allows also the derivation of a CRIT like variable with a
#' different name (ex: `ANL01FL`), without generating additional numeric (ex: `ANL01FN`)
#' and character label (ex: `ANL01`) variables.
#' `r derive_vars_crit::admiralvaccine("deprecated")`
#'
#' @param dataset Input dataset
#' This function is *deprecated*, please use `derive_vars_crit_flag` instead.
#'
#' .
#' @family deprecated
#'
#' @param prefix Variables to add
#'
#' The analysis criterion evaluation variable's name (i.e., `CRIT1`)
#' This name is also used in order to create both character and numeric
#' flags variables (i.e., `CRIT1FL` and `CRIT1FN`).
#' If the name does not contain CRIT wording, it generates a flag variable
#' (ex: `ANL01FL`) whose logic is equals to `CRIT1` variable, without generating
#' additional numeric (ex: `ANL01FN`) and character (`ANL01`) variables.
#'
#'
#' @param crit_label Criterion value
#'
#' A text description defining the condition necessary to satisfy the presence
#' of the criterion
#'
#'
#' @param condition Condition for selecting a subset
#'
#' The condition specified in order to select a subset from the input dataset
#' in which the rule is applied.
#'
#'
#' @param criterion Criterion rule
#'
#' The criterion that each selected row satisfies or not.
#' Returns `Y` or `N` for character variable and `1` or `0` for numeric variable
#' if the criterion is met or not, respectively.
#' Returns `NA` for not selected rows (not taken into account from condition)
#'
#'
#' @return Dataset with criterion variables
#'
#' @export
#'
#' @author Federico Baratin
#' @export
#' @keywords der_var
#' @family der_var
#'
#' @examples
#' library(tibble)
#' library(admiral)
#' library(admiraldev)
#' library(dplyr)
#'
#' input <- tribble(
#' ~USUBJID, ~AVISITN, ~ISCAT, ~PARAMCD, ~AVAL, ~ISLLOQ,
#' "999999-000001", 10, "IMMUNOLOGY", "J0033VN", 2, 4,
#' "999999-000001", 10, "IMMUNOLOGY", "I0019NT", 3, 6,
#' "999999-000001", 10, "IMMUNOLOGY", "M0019LN", 4, 4,
#' "999999-000001", 10, "IMMUNOLOGY", "R0003MA", 3, 6,
#' "999999-000001", 30, "IMMUNOLOGY", "J0033VN", 60, 4,
#' "999999-000001", 30, "IMMUNOLOGY", "I0019NT", 567, 6,
#' "999999-000001", 30, "IMMUNOLOGY", "M0019LN", 659, 4,
#' "999999-000001", 30, "IMMUNOLOGY", "R0003MA", 250, 6,
#' "999999-000002", 10, "IMMUNOLOGY", "J0033VN", 2, 4,
#' "999999-000002", 10, "IMMUNOLOGY", "I0019NT", 7, 6,
#' "999999-000002", 10, "IMMUNOLOGY", "M0019LN", 5, 4,
#' "999999-000002", 10, "IMMUNOLOGY", "R0003MA", 3, 6,
#' "999999-000002", 30, "IMMUNOLOGY", "J0033VN", 55, 4,
#' "999999-000002", 30, "IMMUNOLOGY", "I0019NT", 89, 6,
#' "999999-000002", 30, "IMMUNOLOGY", "M0019LN", 990, 4,
#' "999999-000002", 30, "IMMUNOLOGY", "R0003MA", 340, 6,
#' "999999-000003", 10, "IMMUNOLOGY", "J0033VN", 3, 4,
#' "999999-000003", 10, "IMMUNOLOGY", "I0019NT", 6, 6,
#' "999999-000003", 10, "IMMUNOLOGY", "M0019LN", 2, 4,
#' "999999-000003", 10, "IMMUNOLOGY", "R0003MA", 2, 6,
#' "999999-000003", 30, "IMMUNOLOGY", "J0033VN", 45, 4,
#' "999999-000003", 30, "IMMUNOLOGY", "I0019NT", 381, 6,
#' "999999-000003", 30, "IMMUNOLOGY", "M0019LN", 542, 4,
#' "999999-000003", 30, "IMMUNOLOGY", "R0003MA", NA, 6
#' )
#'
#'
#' derive_vars_crit(
#' dataset = input,
#' prefix = "CRIT1",
#' crit_label = "Titer >= ISLLOQ",
#' condition = !is.na(AVAL) & !is.na(ISLLOQ),
#' criterion = AVAL >= ISLLOQ
#' )
#'
derive_vars_crit <- function(dataset, prefix, crit_label, condition, criterion) {
condition <- assert_filter_cond(enquo(condition))
Copy link

Choose a reason for hiding this comment

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

This is not in line with our deprecation strategy.

Here you have deleted all the function code and just added a warning. So the function behaves completely differently to before, as it is essentially just a warning thrower. If I am using this function in any code and upversion admiralvaccine, now my code will error or at least not derive the variables. Instead, what we want is for the code to still work, but a warning to be thrown (at least for this 1st Phase of deprecation, see again here
for details).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi Edoardo, I can take it. Just one question: on the Phase 1 strategy is suggested to put a warning + replace the "old" function code with the new one. Is this also the case? Or, for the moment, a simple warning + "old" function code is needed?

Copy link

Choose a reason for hiding this comment

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

as you are implementing phase 1, you should just add the warning at the top of the function and then keep all else the same. you should also not delete documentation.

Copy link

Choose a reason for hiding this comment

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

just to add, in your case you don't have any actual new code to replace as you are just deprecating this function in favour of one from another package (admiral)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not in line with our deprecation strategy.

Here you have deleted all the function code and just added a warning. So the function behaves completely differently to before, as it is essentially just a warning thrower. If I am using this function in any code and upversion admiralvaccine, now my code will error or at least not derive the variables. Instead, what we want is for the code to still work, but a warning to be thrown (at least for this 1st Phase of deprecation, see again here for details).

Hi @manciniedoardo Thanks for the review! We have removed the old code and replaced it with a deprecate_warn (maybe it should be replaced with deprecate_inform) and the new admiral::derive_vars_crit_flag. So, I guess the code will still work or am I missing something here?
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, thanks! I have just updated the documentation as per suggestions. @ahasoplakus , I kept the warning you put at the top of the function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ahasoplakus . I just seen that you are working on that. I made a couple of simple updates, so I would stop here just to avoid confusion. Please, let me know if any change is required from my side

Copy link
Collaborator

@ahasoplakus ahasoplakus Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@ahasoplakus . I just seen that you are working on that. I made a couple of simple updates, so I would stop here just to avoid confusion. Please, let me know if any change is required from my side

Thanks @federicobaratin Please feel free to go ahead with the changes @manciniedoardo suggested and fix the checks that are currently failing. I will do a final review and merge

criterion <- assert_filter_cond(enquo(criterion))

var_char <- paste0(prefix, "FL")
var_num <- paste0(prefix, "FN")

if (grepl("CRIT", prefix)) {
data <- dataset %>%
mutate(
`:=`(
!!var_char,
case_when(
!(!!condition) ~ NA_character_,
!!criterion & !!condition ~ "Y",
!(!!criterion) & !!condition ~ "N"
)
),
`:=`(
!!var_num,
case_when(
!(!!condition) ~ NA_real_,
!!criterion & !!condition ~ 1,
!(!!criterion) & !!condition ~ 0
)
),
`:=`(
!!prefix,
case_when(
!(!!condition) ~ NA_character_,
!!criterion & !!condition ~ crit_label,
!(!!criterion) & !!condition ~ crit_label
)
)
)
} else {
data <- dataset %>%
mutate(
`:=`(
!!var_char,
case_when(
!(!!condition) ~ NA_character_,
!!criterion & !!condition ~ "Y",
!(!!criterion) & !!condition ~ "N"
)
)
)
}

return(data)
}
#' @keywords deprecated
#' .
ahasoplakus marked this conversation as resolved.
Show resolved Hide resolved
18 changes: 12 additions & 6 deletions inst/templates/ad_adis.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,13 +322,19 @@ adis <- restrict_derivation(adis,


# STEP 9 Derivation of CRITyFL and CRITyFN ----
adis <- derive_vars_crit(
adis <- restrict_derivation(
dataset = adis,
prefix = "CRIT1",
crit_label = "Titer >= ISLLOQ",
condition = !is.na(AVAL) & !is.na(ISLLOQ),
criterion = AVAL >= ISLLOQ
)
derivation = derive_vars_crit_flag,
args = params(
crit_nr = 1,
condition = AVAL >= ISLLOQ,
description = "Titer >= ISLLOQ",
values_yn = TRUE,
create_numeric_flag = TRUE
),
filter = !is.na(AVAL)
) %>%
arrange(STUDYID, USUBJID, DERIVED, ISSEQ)


# STEP 10 Derivation of TRTP/A treatment variables ----
Expand Down
25 changes: 15 additions & 10 deletions vignettes/adis.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,20 +519,25 @@ dataset_vignette(
Derive Criteria Evaluation Analysis Flags.

The function selects a subset of rows from the input dataset and apply a criterion to them.
If this criterion is met then `CRIT1FL` (or the name you specified in the first argument)
is equal to `Y`; `N` otherwise.
If this criterion is met then `CRIT1FL` is equal to `Y`; `N` otherwise.

The function returns a relative numeric `CRIT1FN` variable (`1` or `0` if the
criterion is met, respectively) and a label `CRIT1` variable
(with the text specified in label_var argument).
criterion is met, respectively) and a label `CRIT1` variable.
```{r eval=TRUE}
adis <- derive_vars_crit(
# STEP 9 Derivation of CRITyFL and CRITyFN ----
adis <- restrict_derivation(
dataset = adis,
prefix = "CRIT1",
crit_label = "Titer >= ISLLOQ",
condition = !is.na(AVAL) & !is.na(ISLLOQ),
criterion = AVAL >= ISLLOQ
)
derivation = derive_vars_crit_flag,
args = params(
crit_nr = 1,
condition = AVAL >= ISLLOQ,
description = "Titer >= ISLLOQ",
values_yn = TRUE,
create_numeric_flag = TRUE
),
filter = !is.na(AVAL)
) %>%
arrange(STUDYID, USUBJID, DERIVED, ISSEQ)
```

```{r, echo=FALSE}
Expand Down
Loading