-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
adding non param methods #25
Conversation
The author of this PR, rimhajal, is not an activated member of this organization on Codecov. |
Maybe the check against R should be in the tests ? I cannot see it yet. |
I am checking it manually for now |
In this case I cannot really help you ^^ But if you have it produce the same numbers, then you are on the right track :) |
seeing that the loops are now different between PoharPerme and EdererI, is it still worth it to merge the lambda function? |
@rimhajal So, at first, I thought that relsurv did simply not implement the summary method for
;) |
@rimhajal missed your question. I dont know, maybe we can implement the other ones (ederer 2, hakulinen), and then decide if we want to merge or not ? |
For the issue with the summary, do we compare with "fit$surv" instead in the tests? |
Yes, compare directly with fit$surv and fit$std.err (while keeping in mind that the time steps are not exactly the same): the summary is j_u_n_k, as we saw friday... |
Do I change the comparison to PoharPerme as well for coherence? |
Hum... Yes that would be better indeed |
The test were already not using the summary, Hakulinen is failing with only the surv rate. |
Is it failling by much ? on the begining or the end of the curve ? Remember the other time we plotted it. |
Can you plot the ratio ? For the moment looks pretty good to me |
Errrr never mind I did not look at the axis correctly. Indeed there is an issue here... |
This is really weird as your implementation looks correct. Maybe we made a mistake in the formula for hakulinen compared to relsurv ? Edit: looking at relsurv code, it looks like somethign reaaaaaaly different is happening for hakulinen : there is a |
The Hakulinen method wasn't in Nathalie's slides, I think we decided this was the formula based on the R code |
Bingo. So thid is not the right formula :P or rather, there is more to it than just this formula. |
if (method == 3) { #need potential follow-up time for Hak. method Any idea what is a "potential follow-up time"? |
Hakulinen Method |
I'd forget a bit about the names and stuff and concentrate on the R code to extract the meaning "later", when we have the right numbers. Looks like the "potential" follow-up time means the death time if not censored and the end-of-study time if censored. Then they use a new censoring indicatrix that says that noone is censored. I dont fully understand why for the moment, sorry |
Looks like the right interpretation is :
And the modification looks like :
This looks a bit crazy to me but it's what is in the code. |
Appendices A.1 to A.4 of this paper from hakulinen himself, in 2011, compares ederer 1 & 2 to hakulinen with clearer-ish formulas. Might be a good source to fix up what we want -- or not. Anyway we could cite it |
The tests are erroring because
|
this doesn't work, pushing just to publish the branch