Enhancements to get_duplicates_dataset()

[thomas-neitmann]

User feedback:

"This function is absolutely cool. It would be great to create a message which dataset is being printed by this function. For example, there may be a situation where you have duplicates in your dataset. Then:

1. you run get_duplicates_dataset()
2. you identify a duplicate
3. you fix a duplicate
4. you run signal_duplicate_records()
5. you no longer see a duplicate
6. you run get_duplicates_dataset()
7. The initial dataset is still being printed.
   It may bring some confusion, so it would be cool to either reset it back to NULL (if the former function hasn't spotted any issue) once the call of a function shows no issues or to simply notify a user which dataset is being printed in the message log. A minor thing. Overall, this is an EXTREMELY useful function and having an ability to quickly spot duplicates by a desired key is BEYOND awesome; "

Based upon that I think we should implement the following:

1. When you call get_duplicated_dataset() information about which call generates this dataset should be printed.
2. The duplicates dataset should be "reset" the next time signal_duplicates_records() is called.

[bundfussr]

Always resetting the duplicates dataset when signal_duplicates_records() is called could corrupt the functionality. Consider the case that a derivation calls signal_duplicates_records() twice. If the first call issues a warning and the second call passes without any issues, get_duplicates_dataset() would not return anything.

What about using a list and a separate reset function. Each time signal_duplucates_records() detects an issue it appends an element to the list. The element could contain the call or description of the context, a timestamp, and the duplicates dataset. By default get_duplicates_dataset() would return the last element. Additionally get_duplicates_dataset() could provide parameters to get other elements of the list.

[thomas-neitmann]

@zdz2101 @sadchla-codes Another interesting issue you might want to tackle.

[zdz2101]

It seems get_duplicates_dataset() is directly tied to signal_duplicates_dataset() and the end goal is to prevent the confusion involved from running get_duplicates_dataset() randomly or pre-emptively before signal_duplicates_dataset(). My proposal would be a warning message if they run get_duplicates_dataset() without calling signal_duplicates_dataset() as a command right before/prior. I have a draft solution for this that I think already passes through the CI/CD checks and it would end up looking something like:

By restricting the use of get_duplicates_dataset() to solely after signal_duplicates_dataset() we don't have to keep track of the variety of changes the user might make along the way fixing the duplicates like renaming objects or having N-amounts of intermediate datasets. get_duplicates_dataset() becomes a tool that returns the duplicates dataframe, if it exists, for a specific dataset we are asking the user to supply. What're your thoughts @thomas-neitmann ?

[thomas-neitmann]

I'm not sure this approach would work in practice. signal_duplicate_records() is usually not called at the top-level as in your example but inside a function (which again might be called fro within another function and so forth). So how would you restrict get_duplicates_dataset() to solely work directly after signal_duplicate_records() is called?

[bundfussr]

I think we can not automate resetting the duplicates dataset or issue a warning or error if it is out of date because we do not know when the user has fixed the program and rerun it.
So we could provide a function for resetting the duplicates dataset but the user would need to call it manually.

[zdz2101]

Within the get_duplicates_dataset() I was playing around and added an if (interactive()) statement that grabs the .Rhistory whenever the function is called, which does a string search for "signal_duplicate_records" from the command prior to the get_duplicates_dataset() call. See lines 38 - 49 , also added lines 135-137 to reset duplicates to NULL if no duplicates were found (for example if they fed in a cleaned/fixed dataset). But if the signal_duplicates_dataset() is wrapped inside other functions, this would indeed lose most of its functionality. Perhaps instead of warning/error, it can still have use as a message to remind user to run signal_duplicates_dataset() or whatever wrapped version of it they're using prior, and string search for functions like derive_vars_merged() as well.

[thomas-neitmann]

User should never run signal_duplicates_records() themselves. That is always called from inside a derive_*() function when something is off to make the user aware of it. Then we tell them to use get_duplicates_datasets() to see the records which caused the issue.

[zdz2101]

After going back to the drafting board for a bit, how does this sound (if user is running in an interactive environment like RStudio):

1. Grabbing/saving user's rhistory during their current session
2. Writing these commands to a temporary .R file
3. Rerunning last command, using source() on the file created from 2
4. Saving the possible error message using sink() to a temporary .txt file
5. Checking if the message contains: "Run get_duplicates_dataset() to access the duplicate records"
6. Message the user that the duplicate records may have been stale, based on what we know of in 5
7. Return the dataset, regardless

Draft available

[bundfussr]

Rerunning user commands looks dangerous to me. It could change the global environment and cause confusion for the user. E.g., if the last command was adlb <- derive_extreme_records(adlb, ...), the new records are duplicated.

[bms63]

Hey @galachad can we get your ideas on this one as well?

[github-actions[bot]]

This issue is stale because it has been open for 90 days with no activity.

[bms63]

Hi all,

@pharmaverse/admiral This issue is stale.

Reviewing the discussion between @zdz2101, @bundfussr and @thomas-neitmann I don't see us implementing this proposal as it seems to invite a lot of potential user-error and confusion. What do you think?

We can move it to a discussion and continue with folks experimenting and discussing and move it up the ladder for discussion at a Core Meeting?

[bundfussr]

Reviewing the discussion between @zdz2101, @bundfussr and @thomas-neitmann I don't see us implementing this proposal as it seems to invite a lot of potential user-error and confusion. What do you think?

I agree. I would close the issue.

[zdz2101]

Agreed I think we can revisit another day if it becomes a popular request

[bms63]

@ddsjoberg @millerg23 @zdz2101 Hi all - Don't forget to put in your ideas from today's meeting around this issue. I was having trouble focusing at the end (lot of ideas being discussed today) and so wasn't able to capture it in my notes.

[millerg23]

A message with the function call to pull out duplicates, would be fine by me. I usually just put the output from get_duplicates() into a data frame, then review it, to see what issue is. So running a different function would be no problem, assume it would be just the underlying function being called in get_duplicates()?

[zdz2101]

Proposal was to give users code to run to create the duplicate object themselves, instead of storing & extracting it for them using a get_ function. Only caveat there is that signal_duplicate_records() is called inside the function body in a handful of places like derive_vars_merged() , derive_vars_obs_number() , which are then called inside a function like derive_vars_joined() so it is inherently a little hard to parse out to begin with

[bundfussr]

I think this works only if the dataset which is checked for duplicates is available to the user. In many cases this is not the case.

By the way, why do you want to provide code for creating the dataset of duplicates instead of providing the dataset directly?

[ddsjoberg]

One way around this would be to print the code to run to see the duplicate values.
(Rather than including this complex call to duplicated(), we could wrap it in a simple fn.)

# create a data frame with duplicates
df <- data.frame(USUBJID = c(letters, letters[1:2]))
columns_to_check <- "USUBJID"

cli::cli_warn(
  c("!" = "Duplicate values were found in columns {.val {columns_to_check}}",
    "i" = "Run {.run df[duplicated(df[{shQuote(columns_to_check)}]), {shQuote(columns_to_check)}, drop = FALSE]} to print the duplicate rows.")
)
#> Warning: ! Duplicate values were found in columns "USUBJID"
#> ℹ Run `df[duplicated(df['USUBJID']), 'USUBJID', drop = FALSE]` to print the
#>   duplcicate rows.

df[duplicated(df['USUBJID']), 'USUBJID', drop = FALSE]
#>    USUBJID
#> 27       a
#> 28       b

^{Created on 2023-08-09 with reprex v2.0.2}

CAVEAT: If we won't the name (ie the symbol) of the data frame we're checking, the code printed would need to replace the data frame name with something the user would need to replace.

[ddsjoberg]

We may be able to just to the .Last.value object, e.g.

# create a data frame with duplicates
df <- data.frame(USUBJID = c(letters, letters[1:2]))
columns_to_check <- "USUBJID"

cli::cli_warn(
  c("!" = "Duplicate values were found in columns {.val {columns_to_check}}",
    "i" = "Run {.run show_duplicates(.Last.value, !!!columns_to_check)} to print the duplicate rows.")
)
#> Warning: ! Duplicate values were found in columns "USUBJID"
#> ℹ Run `show_duplicates(.Last.value, USUBJID)` to print the
#>   duplcicate rows.

show_duplicates(.Last.value, USUBJID)
#>    USUBJID
#> 27       a
#> 28       b

(in this example, the show_duplicates() exists)

[bms63]

Daniel - do you mind making this into a new issue with your suggestion and link it to the discussion? I think you have a lot of balls in the air right now! Lets see if others might be able to implement your proposal.