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.

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

Refactor dropped non-numeric columns with recover #1193

Merged
merged 14 commits into from
May 23, 2024
Merged

Conversation

kylebaron
Copy link
Collaborator

@kylebaron kylebaron commented May 21, 2024

Summary

See #1184

  • This used send a message when important column was dropped due to non-numeric data; the PR refactors the behavior to generate error; I also considered warning, but I'd rather just stop the simulation in this case
  • Throw error rather than message when an important column is dropped; originally aiming for parameters getting dropped, but there will also be error with a dosing column is dropped because it is non-numeric; same for 64 bit integer
  • Refactor recover handling in mrgsim() to defer errors for non-numeric columns that are also parameters; this lets the validate data set functions issue the error
library(mrgsolve)
#> 
#> Attaching package: 'mrgsolve'
#> The following object is masked from 'package:stats':
#> 
#>     filter
library(dplyr)
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union


mod <- house(end = 0)

data <- expand.ev(amt = 100, WTVC = "a")

mrgsim(mod, idata = data)
#> Error in `valid_idata_set()`:
#> ! Found input data that cannot be used for simulation
#> ✖ idata set column: WTVC (character)
#> Backtrace:
#>     ▆
#>  1. └─mrgsolve::mrgsim(mod, idata = data)
#>  2.   └─mrgsolve::mrgsim_i(x, idata = idata, ...)
#>  3.     ├─base::do.call(...)
#>  4.     └─mrgsolve (local) `<fn>`(x = `<packmod>`, data = `<int[,1]>`, idata = `<df[,6]>`)
#>  5.       └─mrgsolve::valid_idata_set(idata, x, verbose = verbose)
#>  6.         └─mrgsolve:::signal_drop(dm, x, to_signal, context = "idata set")
#>  7.           └─rlang::abort(...)

mrgsim(mod, idata = data, recover = "WTVC")
#> Error in `valid_idata_set()`:
#> ! Found input data that cannot be used for simulation
#> ✖ idata set column: WTVC (character)
#> Backtrace:
#>     ▆
#>  1. └─mrgsolve::mrgsim(mod, idata = data, recover = "WTVC")
#>  2.   └─mrgsolve::mrgsim_i(x, idata = idata, ...)
#>  3.     ├─base::do.call(...)
#>  4.     └─mrgsolve (local) `<fn>`(...)
#>  5.       └─mrgsolve::valid_idata_set(idata, x, verbose = verbose)
#>  6.         └─mrgsolve:::signal_drop(dm, x, to_signal, context = "idata set")
#>  7.           └─rlang::abort(...)

Created on 2024-05-21 with reprex v2.0.2

@kylebaron kylebaron requested a review from kyleam May 22, 2024 04:12
Comment on lines +85 to +88
new_col_set <- dimnames(dm)[[2]]
old_col_set <- names(x)
drop <- old_col_set[!old_col_set %in% new_col_set]
drop <- drop[drop %in% check]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated this logic for clarity.

old_col_set <- names(x)
drop <- old_col_set[!old_col_set %in% new_col_set]
drop <- drop[drop %in% check]
if(!length(drop)) return(invisible(NULL))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have to explicitly exit now that errors are generated; I think before, there might have been nothing in drop but we kept going and no one knew.

##' @param verbose logical.
##' @param quiet if `TRUE`, messages will be suppressed.
##'
##' @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.

This is mostly called under the hood; but I filled out details since we're referencing the function name in the error message and probably good to get more of this written down somewhere.

##' An error will be issued when
##' - non-numeric data is found in columns sharing names with model parameters
##' - non-numeric data is found in reserved data items related to dosing
##' (see `mrgsolve:::GLOBALS$CARRY_TRAN`)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is referencing this internal GLOBALS data structure a bad idea? I don't want to have to maintain this list off the contents of what is in there but also feels odd to expose this internal data.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion it's okay to reference. It's useful to those that want to see the list, but it being internal should make it clear that it shouldn't be relied on in scripts, etc.

@@ -582,6 +582,8 @@ do_mrgsim <- function(x,
data$.data_row. <- join_data$.data_row.
carry.recover <- ".data_row."
drop <- names(which(!is.numeric(join_data)))
# Will be dropped with error later when validating data
drop <- drop[!drop %in% c(Pars(x), c(Pars(m), GLOBALS$CARRY_TRAN)]
data <- data[,setdiff(names(data),drop),drop=FALSE]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly can't remember why we're dropping data columns here when it will happen later. For now, will keep it as is.

@kylebaron kylebaron linked an issue May 22, 2024 that may be closed by this pull request
Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

LGTM, left one comment about adjacent doc typo

##' An error will be issued when
##' - non-numeric data is found in columns sharing names with model parameters
##' - non-numeric data is found in reserved data items related to dosing
##' (see `mrgsolve:::GLOBALS$CARRY_TRAN`)
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion it's okay to reference. It's useful to those that want to see the list, but it being internal should make it clear that it shouldn't be relied on in scripts, etc.

R/mrgindata.R Outdated Show resolved Hide resolved
@kylebaron kylebaron merged commit 293a0c9 into main May 23, 2024
5 checks passed
@kylebaron kylebaron deleted the fix/data-drop-recover branch May 23, 2024 18:22
@kylebaron kylebaron mentioned this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

data_set column drop is not reported when item is recovered
2 participants