You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
, 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.
My overall thoughts are that making things easier for users is usually better (unless it causes code to be very difficult to maintain).
So, doing everything nearly as late as possible or redoing work if calls are in the "wrong" order is my main thought. To maintain current functions (and I do see the value in plotting the bins), I think that my ideal would be:
If someone calls predcorrect() then binning(), a predcorrect flag is set in the object and prediction correction could be redone after binning.
I don't know if the original data is stored in a way to allow that, but I think that would be easiest for users. That would also prevent the need for any (or many) warnings.
Hi @billdenney , regarding why we cannot use
predcorrect()
beforebinning()
, the ypc calculation usesmedian(pred)
by bintidyvpc/R/vpcstats.R
Line 597 in 8c7ed9c
predcorrect()
, first,stratbin
isNULL
and the ypc calculation then incorrectly usesmpred
value asmedian(pred)
for the entire data. We did have a check for this but it was erroneously commented out when I made changes to allowbinless()
to be used before/afterpredcorrect()
.If using
predcorrect()
+binning()
, we could simply usepredcorrect()
to update thetidyvpcobj
with required attributes e.g.,predcor
and actually perform theypc
calculation insidevpcstats()
, this would allow users to specify the functions in whatever order (this is the case forpredcorrect()
+binless()
when calculatingl.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 whenpredcorrect()
is used, the resulting points on the plot are theobs$ypc
values.I've made some changes to
predcorrect()
usage as I described to you, deprecating additional usage ofloess.ypc
argument insidebinless()
, nowbinless()
+predcorrect()
will automatically perform LOESS pcVPC without needing additional redundant argument. I also now ensurebinning()
must be performed beforepredcorrect()
if performing traditional pcVPC. Users can still callbinless()
afterpredcorrect()
for backwards compatibility, but they will now receive a warning. See PR: #42I'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.
Originally posted by @certara-jcraig in #36 (comment)
The text was updated successfully, but these errors were encountered: