ronkeizer / vpc

R library to create visual predictive checks (VPC)
Other
36 stars 20 forks source link

vpc function as an S3 method? #46

Closed mattfidler closed 6 years ago

mattfidler commented 6 years ago

Hi Ron,

Thank you for the excellent package. I was hoping to integrate this into nlmixr. For nlmixr you can simulate the required information and then call this function.

In that regard, it would be convienient to call:

vpc(nlmixr.fit)

And output the vpc (using your excellent package of course). All that would be required is the vpc be an S3 method.

I could submit a pull-request if you think this is a good idea.

Also, I'm not sure what the software option does, but perhaps we could allow nlmixr be a software option?

ronkeizer commented 6 years ago

hi Matt, sounds great, happy to help make this work. The software option can be used to specify the type of input data. Currently it only recognizes NONMEM and Phoenix, and just sets the column name defaults (ID, PRED, etc) used when parsing the supplied obs and sim data. I believe it is now set to auto so it will detect automatically. You could indeed create a vpc.nlmixr() S3 function, perhaps it should then just call the main vpc function with the "nlmxir" software argument? Was that what you were thinking? Also, does a nlmixr.fit object already contain both observed and simulated data? I would prefer to keep the vpc library as only a parser/plotter of data, so ideally it shouldn't have to invoke any function in nlmixr to create the simulated data. Happy to discuss and/or look at PR.

mattfidler commented 6 years ago

Hi Ron,

Thank you for explaining the software option.

I already have a very small code that is in nlmixr to show how it could work:

https://github.com/nlmixrdevelopment/nlmixr/blob/master/R/vpc.ui.R

Basically, you would specify the fit and the number of simulations and you are good to go. The number of simulations would default to 100. These would take the place of sim and obs arguments.

If I changed this, we would be able to just do

vpc(fit,1000)

And it would plot the fit!

I will send a pull request shortly.

mattfidler commented 6 years ago

If you want I could also update the documentation to say:

For nlmixr

This way you could also pipe, if you really wanted:

fit %>% vpc(1000)
mattfidler commented 6 years ago

I don't think the software option needs to be updated.

mattfidler commented 6 years ago

I forgot you could send options to RxODE like the number of cores for threaded-solving, so if it is OK, I would also like to add the ... argument as well.

ronkeizer commented 6 years ago

fine with me to add the ... argument, please put in a new PR and I'll look at it asap. Regarding sim and obs as suggested above, this seems to conflict with the argument names vpc() is already using, no? Or perhaps I misunderstand what you mean here.

mattfidler commented 6 years ago

Ok. I can update this pull request and you can squash and merge if you only want one merge.

Regarding sim and obs as suggested above, this seems to conflict with the argument names vpc() is already using, no? Or perhaps I misunderstand what you mean here.

Yes it conflicts. I am fine leaving them as they are.

In the other vpc function I wanted to have the fit as the first argument; Since sim is the first argument to maintain argument alignment with the s3 method, I would be using this name, though I do not have to do that. However, I get a R CMD CHECK warning otherwise

mattfidler commented 6 years ago

Nevermind I didn't notice you already merged it; My mistake :)

mattfidler commented 6 years ago

The other option is to use a generic function for the s3 class

vpc <- function(...){
}

And state that the documentation of each of the methods is in their respective classes, though this is a bit of a pain.

mattfidler commented 6 years ago

I submitted a pull request

mattfidler commented 6 years ago

Thanks; Let me know when you submit this to CRAN again.