Closed leonardommarques closed 6 years ago
Thanks Leonardo - looks great. Just merged it into the master branch with some minor edits.
@leonardommarques Can I just clarify why you do
med_from_start = start_p * (1-t)
in summary.fns
? This seems backwards to me - e.g. the 10% quantile of the survival time distribution is returned for quantiles = 0.9
. See issue #57. I know in survival analysis we usually work in terms of survivor functions rather than CDFs, but a quantile is usually defined from P(X < q) not P(X > q).
@chjackson , I agree that quantiles are usually defined from P(X < q). I implemented this function thinking about an inverse Survival function. If I understood correctly, in case we change it to match P(X < q) we might run into some inconsistencies. Check the issue #57:
In summary, I think it is good that the function keeps the current behavior, especially if the amount of downloads after this implementation is high. But I understand it is good to agree with the standard definitions and with the comunity. Plus, it is your package and I trust you know what is the best to do.
Perhaps an optional argument to reverse consistent with the way the p-functions work?
From: Leonardo notifications@github.com Sent: Tuesday, October 23, 2018 3:10 PM To: chjackson/flexsurv-dev Cc: Subscribed Subject: Re: [chjackson/flexsurv-dev] Add "quantile" as an option for the type of summary.flexsurvreg (#49)
@chjacksonhttps://github.com/chjackson , I agree that quantiles are usually defined from P(X < q). I implemented this function thinking about an inverse Survival function. If I understood correctly, in case we change it to match P(X < q) we might run into some inconsistencies. Check the issue#57https://github.com/chjackson/flexsurv-dev/issues/57:
In summary, I think it is good that the function keeps the current behavior, especially if the amount of downloads after this implementation is high. But I understand it is good to agree with the standard definitions and with the comunity. Plus, it is your package and I trust you know what is the best to do.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/chjackson/flexsurv-dev/pull/49#issuecomment-432380597, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AKbbr_LhNnyGQBQkUvGMlk_78SxFBNfTks5un2m_gaJpZM4U9trJ.
I think it's currently inconsistent - the function returns the mean and median of the distribution of survival times. So it's fairly clear to me that type="quantile" with quantiles=0.1 should return the 10% quantile of the distribution of survival times X, that is the value q for which P(X<q) = 0.1. Also this feature is not released on CRAN yet so it's fine to change I think.
Changed in [8e6cc7b]
Fix issue #48
I have included the
type="quantile"
and thequantiles
parameter for summary.flexsurvreg.I have also written two test functions:
1) compare the results of
type="quantile", quantiles=0.5
andtype="median"
. 2) expect an error ifquantiles
is not in between 0 and 1.I was asked to reimplement the "median" option as a special case of your "quantile". I think that this reimplementation will not be backwards compatible, since
type="median"
returns only three columns andtype="quantile"
returns four. I am afraid some users are implementing something likesummary(..., type="median")[1,1]
. But I can do it if you think it is fot the best.