Open Vincent-Maladiere opened 1 month ago
The CI for the doc fails because the previous boosting tree model is missing. This should be fixed when https://github.com/soda-inria/hazardous/pull/53 is merged.
Our implementation is 100x slower than scikit-survival concordance_index_ipcw
. This is due to the weight computing (the IPCWs) inside the BalancedTree, which lifelines doesn't perform.
On a dataset with 20k rows:
our implementation: 18.10s
scikit-survival: 0.24s
lifelines: 0.27s
The flamegraph is quite clear about the culprit, being the list comprehension that computes the IPCW weight for each pair. When I remove the IPCWs, the performance becomes similar to lifelines.
I tried to fix this performance issue using numba @jitclass on the BTree, but it is still very slow. I put the numba BTree on a separate draft branch for reference.
I only see two ways forward:
Pinged by @Vincent-Maladiere, but have no time for it.
Random pile of pieces of advice:
pydatastructures
No, don't use compiled languages, please. It will make release and distribution much harder.
On Jul 26, 2024, 13:46, at 13:46, Julien Jerphanion @.***> wrote:
Pinged by @Vincent-Maladiere, but have no time for it.
Random pile of pieces of advice:
- find if a better algorithm exist first
- profile to see what's the bottleneck
- see if tree-based structures can be used from another library (e.g.
pydatastructures
- use another language (like Cython or C++) to implement the costly algorithmic part
-- Reply to this email directly or view it on GitHub: https://github.com/soda-inria/hazardous/pull/71#issuecomment-2252581805 You are receiving this because you were mentioned.
Message ID: @.***>
After giving it some more thought, there is room for improvement with the current balanced tree design :
However, when we use a conditional IPCW estimator (like Cox or SurvivalBoost), we have: $$W_{ij,1} = \hat{G}(T_i | X_i) \hat{G}(T_i | Xj) \space \mathrm{and} \space W{ij,2} = \hat{G}(T_i | X_i) \hat{G}(T_j | X_j)$$
In this case, the balanced tree is not adapted anymore, and we should use the naive implementation.
So, to make things simpler, I suggest we only implement the naive version for now, and eventually return to the balanced tree later, for the non-conditional and unweighted cases.
WDYT?
Sounds good to me. We can always iterate if needed
On Jul 26, 2024, 18:38, at 18:38, Vincent M @.***> wrote:
After giving it some more thought, there is room for improvement with the current balanced tree design :
- When we don't use an IPCW estimator (like lifelines): $$W{ij,1} = W{ij,2} = 1$$
- When we use a non-conditional IPCW estimator (Kaplan-Meier, like scikit-survival): $$W{ij,1} = W{i,1} = \hat{G}(Ti) ^ 2 \space \mathrm{and} \space W{ij,2} = \hat{G}(T_i) \hat{G}(T_j) $$
However, when we use a conditional IPCW estimator (like Cox or SurvivalBoost), we have: $$W_{ij,1} = \hat{G}(T_i | X_i) \hat{G}(T_i | Xj) \space \mathrm{and} \space W{ij,2} = \hat{G}(T_i | X_i) \hat{G}(T_j | X_j)$$
In this case, the balanced tree is not adapted anymore, and we should use the naive implementation.
So, to make things simpler, I suggest we only implement the naive version for now, and eventually return to the balanced tree later, for the non-conditional and unweighted cases.
WDYT?
-- Reply to this email directly or view it on GitHub: https://github.com/soda-inria/hazardous/pull/71#issuecomment-2253113408 You are receiving this because you were mentioned.
Message ID: @.***>
Here is the revised version. When used on a survival dataset, it gives identical results to scikit-survival, with a slightly better time complexity.
This PR is now ready to be reviewed :)
What does this PR propose?
This PR proposes to add the c-index as defined in [1]. I think this is ready to be reviewed for merging, with some questions/suggestions in the TODO section below.
show maths
concordance_index_incidence
function is inspired by theconcordance_index
function in lifelines, with some significant differences:concordance_index_ipcw
._BTree
class fromlifelines.utils.btree.py
by adding a weighting count mechanism. I referenced lifelines inhazardous.metrics._btree.py
, but I can reference it also in thehazardous.metrics._concordance_index.py
file if necessary.TODO
tied_tol
parameter for ties in predictions?cc @ogrisel @GaelVaroquaux @juAlberge @glemaitre
[1] Wolbers, M., Blanche, P., Koller, M. T., Witteman, J. C., & Gerds, T. A. (2014). Concordance for prognostic models with competing risks.