InsightRX / mipdtrial

Tools for simulating model-informed precision dosing trials
https://insightrx.github.io/mipdtrial/
Other
2 stars 0 forks source link

Updates to move busulfan and PEG-Asp to new framework #11

Closed jasmineirx closed 3 weeks ago

jasmineirx commented 3 weeks ago

Still some clean-up to be done to get this merged but I thought this was a reasonable amount to review to start.

mccarthy-m-g commented 3 weeks ago

Do you mind if I commit some revisions to the vignette?

jasmineirx commented 3 weeks ago

sure! i'm working on getting some of the unit tests working right now. They're not failing due to change I made here. it's due to other issues in the base feature branch.

mccarthy-m-g commented 3 weeks ago

I made some small changes to prose and code for better flow. I also added a few FIXME comments to the vignette that you might want to address.

Regarding the internal data set FIXME comment: If you haven't added a data set to a package before, let me know and I'd be happy to do it!

jasmineirx commented 3 weeks ago

I'm a bit hesitant to add a data set to a package for only one vignette. wdyt?

mccarthy-m-g commented 3 weeks ago

That's one of the most common purposes of adding data sets to a package 😄 I do think it would make the vignette a lot easier to read---the wrangling code there distracts from the purpose of what you're demonstrating.

It takes little effort, no maintenance, and just a small bit of documentation. We basically just need to move the wrangling code you've already done to a script in a data-raw directory, which will call usethis::use_data() on the final data set. Then just add an R script with roxygen documentation for the data and a pkgdown reference.

jasmineirx commented 3 weeks ago

Hey, changed to predefining the data using constructive::construct and removed the NHANES Suggests, since this seems to balance simplicity and readability well.

Tests are still failing but I'll look into it tomorrow.