search

metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
23 stars 2 forks source link

nm_table_files: use nmrec instead of regex #603

Closed barrettk closed 10 months ago

barrettk commented 1 year ago

Addresses the second bullet of issue https://github.com/metrumresearchgroup/bbr/issues/599

barrettk commented 10 months ago

My main comment (inline) is that the proposed change will error if every table record doesn't have a file option (which isn't required by NONMEM).

My main question here is: What is the expected result of nm_table_files in these cases? If no filename is specified, we currently wouldnt look for that table and by extension, join that data in nm_join. Is it expected that we ignore these tables if no filename is specified?

kyleam commented 10 months ago

Is it expected that we ignore these tables if no filename is specified?

My read of the NONMEM docs is that, if no filename is specified, the output is just printed in the NONMEM output. So, yes, I'd say the only thing to do is ignore such table records.

seth127 commented 10 months ago

the only thing to do is ignore such table records

This makes sense to me. It also made me think about what we do if there aren't any table files. The default path looks like it errors out (here). Passing NULL (or anything else that doesn't lead to actual files on disk) looks like it will error out down the line somewhere (either here or when it's passed to nm_file() here).

Either way, I think that makes sense to error out if no table files are found. In this case, there's no value in nm_join() and the caller could just use nm_data() directly.

As an aside: I was looking into it because I was considering what to do for this "no table files found" scenario in https://github.com/metrumresearchgroup/bbr.bayes/issues/96. That gets a bit more complicated (because of the simulations downstream), but that discussion doesn't belong here.

barrettk commented 10 months ago

Either way, I think that makes sense to error out if no table files are found. In this case, there's no value in nm_join() and the caller could just use nm_data() directly.

@seth127 Yeah this should be taken care of here

barrettk commented 10 months ago

FYI we need to run devtools::document() in the parent branch before merging. Forgot to generate the man file for the new function before committing/merging

kyleam commented 10 months ago

FYI we need to run devtools::document() in the parent branch before merging.

Okay. So it doesn't get forgotten, I'd say it's fine for you to do that now and push directly to dev-nmrec.