nlmixr2 / babelmixr2

The goal of babelmixr2 is to interface nlmixr2 with other pharmacometric tools
https://nlmixr2.github.io/babelmixr2/
8 stars 1 forks source link

Use `system2()` instead of `system()`? #23

Closed billdenney closed 2 years ago

billdenney commented 2 years ago

Using system() will likely result in issues with spaces in filenames and parsing of arguments. system2() has the complexity that it requires using a vector of command line arguments instead of a simpler string. This issue is mainly for consideration rather than trying to push hard for system2()-- it's more of a gentle nudge.

https://github.com/nlmixr2/babelmixr2/blob/30e976377db64b3ad41e6cb0fc06ec1ad12db478/R/nonmemNlmixr2est.R#L209

The same would apply for Monolix:

https://github.com/nlmixr2/babelmixr2/blob/3144dbed1226602b684f1930460bacca15da38d5/R/monolixNlmixr2est.R#L230

billdenney commented 2 years ago

Actually, I have a more thorough thought for this. Could the option either be a character string which is assumed to work as a path to nmfe75 or an R function that takes two arguments, the model filename and the output filename and then is responsible for running NONMEM? The benefit of the second method is it allows much more complexity in running NONMEM (like grid submission) without it being directly integrated or supported by babelmixr2.

mattfidler commented 2 years ago

This is fine with me. Make the change if you want, though I think in Monolix if you have lixsoftConnectors it will work anyhow.

billdenney commented 2 years ago

Do we need to wait for a NONMEM run if the run command is not setup? I'm wondering if a simplification of the runner code could simply stop without running and tell the user, "run NONMEM manually and rerun nlmixr() or setup NONMEM's run command".

Then, we could drop all of the waiting part.