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

Cox model: c-index #466

Open
EQUIWDH opened this issue Dec 29, 2022 · 7 comments · Fixed by #477
Open

Cox model: c-index #466

EQUIWDH opened this issue Dec 29, 2022 · 7 comments · Fixed by #477
Assignees
Labels
enhancement New feature or request

Comments

@EQUIWDH
Copy link

EQUIWDH commented Dec 29, 2022

Hello, I 'm using the package to calculate some real survival data, I find the cross-validation can only choose deviance on the test cohort to determine the support size. could you guys add the c_index principle for cox model's cross validation?

@Mamba413
Copy link
Collaborator

@oooo26 ,I think we can first work on classification with auc, and then, turn to this issue.

@Mamba413 Mamba413 added the enhancement New feature or request label Jan 16, 2023
@oooo26
Copy link
Collaborator

oooo26 commented Jan 17, 2023

No prob, I will take a look.

@Mamba413 Mamba413 changed the title Cross Validation Principle of Cox model Cox model: c-index Jan 17, 2023
@Mamba413
Copy link
Collaborator

definition of C-index: https://statisticaloddsandends.wordpress.com/2019/10/26/what-is-harrells-c-index/.
I also recommend implementing C-index following some mature libraries like ranger: https://github.com/imbs-hl/ranger/blob/master/src/TreeSurvival.cpp.

@Mamba413 Mamba413 linked a pull request Jan 29, 2023 that will close this issue
@oooo26 oooo26 reopened this Jan 31, 2023
@EQUIWDH
Copy link
Author

EQUIWDH commented Mar 21, 2023

It seems the whole constructure do not provide a parameter space for time vector when the situation is censored ,because the T1 *y for cox model only pass the censoring indicator sorted by time column to calculate loss enough. However, If hope to calculate Harrel's c-index for censored data especially when $T_i = T_j$ shows up. The time shown in bess_base.py must be passed for the initialization of abessCox in AlgorithmGLM.h. I find other file defaults y is a vector so I guess I can not directly regard it as a n*2 matrix to pass the parameter. Hence, the other way is adding an initialization parameter for time. could u take a look how to do it because I think I must update many other files and it's a little bit tough.

@Mamba413
Copy link
Collaborator

@oooo26 ,what about add a parameter for API?

@oooo26
Copy link
Collaborator

oooo26 commented Mar 22, 2023

Actually modifying the API is easy, but as you discussed, I think the trouble is how to use time in CV, e.g.:

  • If stacking time with y, you need to change the Cox model into multiple one;
  • If storing time into Data, you need to modify CV splitting process and pass it to score computation;
  • If adding time as a seperated/subclass variable, more functions need to be changed and it would be annoying;

I would recommend the second way. But indeed, each of them requires updates on many files, so if there is a better idea, please do it.

@EQUIWDH
Copy link
Author

EQUIWDH commented Mar 23, 2023

Ok, thanks for the recommendation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

3 participants