Add nlmixr2 vignette

[billdenney]

Fix #27

Note that there are some issues that appear to be in play within tidyvpc that occurred during creating this vignette. Those will be addressed separately.

[codecov-commenter]

Codecov Report

Merging #36 (a833c38) into master (8c7ed9c) will not change coverage.
The diff coverage is n/a.

❗ Current head a833c38 differs from pull request most recent head b185fcb. Consider uploading reports for the commit b185fcb to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master      #36   +/-   ##
=======================================
  Coverage   80.20%   80.20%           
=======================================
  Files           4        4           
  Lines        1051     1051           
=======================================
  Hits          843      843           
  Misses        208      208           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[billdenney]

I'm working to fix the pred-corrected example.

[billdenney]

@certara-jcraig, I'm having trouble with the pred-corrected VPC in the last section of the vignette. Can you please take a look and see if you can tell what I'm missing?

[certara-jcraig]

@certara-jcraig, I'm having trouble with the pred-corrected VPC in the last section of the vignette. Can you please take a look and see if you can tell what I'm missing?

Hi @billdenney, the predcorrect() should be called after binning() : https://certara.github.io/tidyvpc/articles/tidyvpc_cont.html#predcorrect

vpc_predcorr <-
  observed(fit$dataMergeLeft %>% filter(EVID == 0), x=TIME, y=DV) %>%
  simulated(fit_vpcsim, x=time, y=sim) %>%
  stratify(~ WTSTRATA) %>%
  binning(bin = "jenks") %>%
  predcorrect(pred=PRED) %>%
  vpcstats()

I acknowledge that this may lead to confusion and at minimum should be better documented. I'll have some time to more thoroughly review the implementation this weekend, and pretty confident that we should be able to support predcorrect() before/after binning, or at least we should warn/error users if they specify out of order.

Also noting that in earlier versions of tidyvpc, predcorrect() was required to be specified before the call to binless(), but in 1.4.0 release, I changed implementation to support predcorrect() usage either before or after binless(). I see in the vignette we are still stating that predcorrect() should be called before binless() though. I will need to update this.

[certara-jcraig]

Hi @billdenney , regarding why we cannot use predcorrect() before binning(), the ypc calculation uses median(pred) by bin

tidyvpc/R/vpcstats.R

Line 597 in 8c7ed9c

mpred <- mpred[, mpred := median(pred), by=stratbin]

, and in your pcVPC when you specified predcorrect(), first, stratbin is NULL and the ypc calculation then incorrectly uses mpred value as median(pred) for the entire data. We did have a check for this but it was erroneously commented out when I made changes to allow binless() to be used before/after predcorrect().

If using predcorrect() + binning(), we could simply use predcorrect() to update the tidyvpcobj with required attributes e.g., predcor and actually perform the ypc calculation inside vpcstats(), this would allow users to specify the functions in whatever order (this is the case for predcorrect() + binless() when calculating l.ypc). However, the reason we don't do this is because users can actually plot the bins without the vpc statistics (https://certara.github.io/tidyvpc/articles/tidyvpc_cont.html#visualize-bins), and when predcorrect() is used, the resulting points on the plot are the obs$ypc values.

I've made some changes to predcorrect() usage as I described to you, deprecating additional usage of loess.ypc argument inside binless(), now binless() + predcorrect() will automatically perform LOESS pcVPC without needing additional redundant argument. I also now ensure binning() must be performed before predcorrect() if performing traditional pcVPC. Users can still call binless() after predcorrect() for backwards compatibility, but they will now receive a warning. See PR: #42

I'm not too happy about the additional warnings and implementation in general, but this is required to preserve existing behavior. We may decide to overhaul some of this for the next major version release. Happy to hear your thoughts.

[billdenney]

I just realized that having the vignette work with nlmixr2 will require suggesting the nlmixr2 package. That is a heavy suggested package. If you know a way to make it more optional, I'd be happy to incorporate that change.

[certara-jcraig]

I just realized that having the vignette work with nlmixr2 will require suggesting the nlmixr2 package. That is a heavy suggested package. If you know a way to make it more optional, I'd be happy to incorporate that change.

Hmm, instead of adding to Suggests, what about adding in an additional step inside check-standard.yml to run install.packages("nlmixr2") e.g.,

 # Install vignette dependencies
 - name: Install nlmixr2
    run: |
          install.packages("nlmixr2")

I haven't done this before however, and I expect the vignette will not be able to build on CRAN without nlmixr2 listed in suggests. We can still deploy the vignette to pkgdown website though. I would be OK with ultimately adding it to suggests, but we'll also need to assess whether CRAN will complain about overall R CMD check time duration.

[billdenney]

I think that the package being used in a vignette without being in suggests will fail CRAN.

I don't think that the package check time on CRAN will be affected since I think that you get the package installation time on CRAN for free (for other packages). Because I think that the CRAN infrastructure includes all packages. (I'm not certain of that, though.)

[certara-jcraig]

Thanks @billdenney, merging PR.