metrumresearchgroup / mrgsolve

Simulate from ODE-based population PK/PD and QSP models in R
https://mrgsolve.org
GNU General Public License v2.0
130 stars 36 forks source link

Minor Bug: matlist objects can have duplicate labels #692

Closed billdenney closed 4 years ago

billdenney commented 4 years ago

This is related to #690 and #691.

It seems like the labels attribute of matlist objects should be required to be globally unique. In the example below, any(duplicated(unlist(new_omegalist@labels))) should be required to be FALSE. If something like #691 is accepted, then I think it should become an error for them to be duplicated, and if that path is not chosen, then I think it should become a warning to alert the user that they have probably done something unintentional.

library(mrgsolve)
#> 
#> Attaching package: 'mrgsolve'
#> The following object is masked from 'package:stats':
#> 
#>     filter
new_omegalist <- c(omat(house()), omat(house()))
new_omegalist
#> $...
#>         [,1] [,2] [,3] [,4]
#> ECL:       0    0    0    0
#> EVC:       0    0    0    0
#> EKA:       0    0    0    0
#> EKOUT:     0    0    0    0
#> 
#> $...
#>         [,1] [,2] [,3] [,4]
#> ECL:       0    0    0    0
#> EVC:       0    0    0    0
#> EKA:       0    0    0    0
#> EKOUT:     0    0    0    0
dput(new_omegalist)
#> new("omegalist", data = list(... = structure(c(0, 0, 0, 0, 0, 
#> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0), .Dim = c(4L, 4L), .Dimnames = list(
#>     c("1: ", "2: ", "3: ", "4: "), NULL)), ... = structure(c(0, 
#> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0), .Dim = c(4L, 4L
#> ), .Dimnames = list(c("1: ", "2: ", "3: ", "4: "), NULL))), n = c(4L, 
#> 4L), labels = list(c("ECL", "EVC", "EKA", "EKOUT"), c("ECL", 
#> "EVC", "EKA", "EKOUT")))

Created on 2020-06-16 by the reprex package (v0.3.0)

kylebaron commented 4 years ago

Thanks, Bill. There is a check in the model object that will stop you if you create duplicate labels. But that's downstream of the matlist object. I'll put the check in place for the validity check for that object.

kylebaron commented 4 years ago

Formally addressed in #747