Skip to content
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

Survival measures #1372

Closed
wants to merge 26 commits into from
Closed

Survival measures #1372

wants to merge 26 commits into from

Conversation

mllg
Copy link
Sponsor Member

@mllg mllg commented Dec 5, 2016

No description provided.

@mllg
Copy link
Sponsor Member Author

mllg commented Dec 5, 2016

Now each survival model has the the training survival times attached (required for cindex.uno and more measures I'll implement).

The time-dependent measures sometimes return NaN/NA and I think this is related to the max.time stuff you were discussing. If we cannot set a good default here, we probably should force the user to set one himself (i.e., initialize with NA). Thoughts?

@mllg
Copy link
Sponsor Member Author

mllg commented Dec 5, 2016

@PhilippPro
Copy link
Member

PhilippPro commented Dec 5, 2016

Ok, there is also my wished integrated AUC.

@mllg
Copy link
Sponsor Member Author

mllg commented Dec 5, 2016

Added iauc.uno. AUC.sh does not work for me, I get segfaults :/

I think this is more user friendly, similar to the cost measure constructor. Of course we should delete the other measures, if we use this one.
@studerus
Copy link
Contributor

studerus commented Dec 7, 2016

Good idea to make a constructor for time dependent AUC measures!

@mllg
Copy link
Sponsor Member Author

mllg commented Dec 8, 2016

I've cleaned it up a little. I'm still unsure whether I like the constructor or not...

@berndbischl
Copy link
Sponsor Member

I've cleaned it up a little. I'm still unsure whether I like the constructor or not...

what do you dislike? (i suggested that to Phillip)
that seems to be the best option to make the measure, an object, parameter dependent

@PhilippPro
Copy link
Member

"Philipp" please. And if you look here you could also look in the PR for getOOB. ;)

@berndbischl
Copy link
Sponsor Member

we need a spitzname for you

@mllg
Copy link
Sponsor Member Author

mllg commented Dec 13, 2016

what do you dislike? (i suggested that to Phillip)

I don't see an advantage over a simple and generic setMeasurePars which then can be reused for future measures which come with parameters. Furthermore, measures which come with a special constructor are not documented in ?measures, right?

@berndbischl
Copy link
Sponsor Member

I don't see an advantage over a simple and generic setMeasurePars which then can be reused for future measures which come with parameters. Furthermore, measures which come with a special constructor are not documented in ?measures, right?

ok thats a good point. i do have the feeling that we are invading this PR a bit though. new issue?
also we have one other old measure that this relates to, makeCostMeasure.

@PhilippPro
Copy link
Member

What's the status here? Until we have setMeasurePars, we use the constructor?

@PhilippPro
Copy link
Member

@studerus: We decided to delete three of the measures for time dependent auc, because they are numerically unstable. They provided results smaller than 0 and many times where NA, so they did not pass our tests. They are now in the todo-files folder, where they can potentially be fixed.

Do you maybe know which of these four time dependent auc measures is the best one?

@PhilippPro
Copy link
Member

By the way, we are thinking of writing a small paper about it (maybe after integrating also the ibs, would you like to join?

@mllg
Copy link
Sponsor Member Author

mllg commented Mar 19, 2017

Might be ready for merge. We will probably add more measures after this PR (we moved some to todo-files). Still no working solution for brier score.

@studerus
Copy link
Contributor

studerus commented Mar 21, 2017

@PhilippPro
It's unfortunate that you had to take out these measures. According to this simulation study the kernel weighting method as implemented in the tdROC package would be the best time dependent ROC measure. What exactly was the problem with the measures?

Writing a paper is good idea and I would be happy to join.

@studerus
Copy link
Contributor

@mllg
What is the problem with the brier score? I thought we had solved the problem after our email discussion with Gerds.

@PhilippPro
Copy link
Member

The problem was that in our tests the measures provided errors many times. See my message above:

They provided results smaller than 0 and many times where NA, so they did not pass our tests.

One problem might be, that you cannot set time higher than the highest time point, but in resampling this can happen quite easily.

@PhilippPro
Copy link
Member

PhilippPro commented Mar 21, 2017

With the brier score we were unhappy that in the implementations this special kind of object is needed. We also wanted to do that in a separate new commit and either use pec or (my preferential but labourious option) code it by ourselves.

@studerus
Copy link
Contributor

@PhilippPro

Did you also inform the developers of tdROC package about this problem?

@PhilippPro
Copy link
Member

No. They probably also did not plan to use it for resampling. ;) But we could maybe write them.

@lintr-bot
Copy link

R/measures.R:1405:35: style: Commas should always have a space after.

timeROC::timeROC(T = truth[,1L], delta = truth[,2L], marker = response, times = max.time,
                                  ^

R/measures.R:1405:55: style: Commas should always have a space after.

timeROC::timeROC(T = truth[,1L], delta = truth[,2L], marker = response, times = max.time,
                                                      ^

tests/testthat/test_learners_all_surv.R:12:100: style: Commas should always have a space after.

sub.task = subsetTask(surv.task, subset = c(1:70), features = getTaskFeatureNames(surv.task)[c(1,2)])
                                                                                                   ^

@PhilippPro
Copy link
Member

We are somehow stuck here. How is the actual status, do we need #1742?
I also do not understand the failed travis...

My supervisor showed me this paper (https://bmcmedresmethodol.biomedcentral.com/articles/10.1186/s12874-017-0336-2), where they recommend the Uno-Index.

@mllg
Copy link
Sponsor Member Author

mllg commented Jun 9, 2017

I rewrote stuff of this PR in the survival branch in a clean way. I've discovered multiple issues, I'll get back to them in the new PR.

@mllg mllg closed this Jun 9, 2017
@mllg mllg mentioned this pull request Jun 9, 2017
@mllg mllg deleted the survival_measures branch July 17, 2017 20:16
@mllg mllg restored the survival_measures branch July 17, 2017 20:17
@pat-s pat-s deleted the survival_measures branch December 31, 2019 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants