-
Notifications
You must be signed in to change notification settings - Fork 52
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
CoxPHLoss does not handle batches where all samples are censored #52
Comments
Hello, can you produce a minimal reproducible example?
wouldn't it be one? Can you test with the Breslow approximation? |
Sorry for the late response. A minimal example would be:
The error for the Efron method happens because of what I've described above. I think the |
I'll look at it tonight. But an empty product is 1, not 0, hence the log
will be 0.
Le lun. 8 janv. 2024, 09:57, lenbrocki ***@***.***> a écrit :
… Sorry for the late response. A minimal example would be:
import torch
from lassonet.cox import CoxPHLoss
loss = CoxPHLoss("breslow")
labels = torch.tensor([[5.0, 0], [2.0, 0]])
hazards = torch.tensor([5.0, 2.0])
print(loss(hazards, labels))
#prints nan
loss = CoxPHLoss("efron")
labels = torch.tensor([[5.0, 0], [2.0, 0]])
hazards = torch.tensor([5.0, 2.0])
print(loss(hazards, labels))
#fails with RuntimeError: max(): Expected reduction dim to be specified for input.numel() == 0. Specify the reduction dim with the 'dim' argument.
The error for the Efron method happens because of what I've described
above. I think the nan in the breslow case happens because the likelihood
is zero. This can for example be seen from your paper
https://arxiv.org/pdf/2208.09793.pdf in equation 1: if all $\delta_i$ are
zero, the product is zero and then the log of this is not defined.
—
Reply to this email directly, view it on GitHub
<#52 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADEQQFJM3G6QQAZPKXGMPF3YNOYIRAVCNFSM6AAAAABA6M7RZ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBQGU4TQNJYGM>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Oh I wasn't aware of that, but yes you're right of course. Then the problem becomes why |
I tried a fix, can you test it?
Le lun. 8 janv. 2024, 10:37, lenbrocki ***@***.***> a écrit :
… Oh I wasn't aware of that, but yes you're right of course. Then the
problem becomes why nan is returned and not 0.
—
Reply to this email directly, view it on GitHub
<#52 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADEQQFIFJVKAUTX3FGRJWXTYNO44BAVCNFSM6AAAAABA6M7RZ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBQGY2TIOBWHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
The CoxPHLoss now correctly returns 0. But when I'm trying to use the fixed loss in training I'm getting for batches where all samples have
|
I think I managed to find a better fix :) Can you test again? |
Sorry again for the delay. Yes, it's working now! |
Great! |
I'm training a custom model with the
CoxPHLoss
and have noticed that when using the Efron tie method the training will fail when a batch only contains censored events. The code giving the error is in lassonet/utils.py:When all samples are censored
index
will be an empty tensor andindex.max()
fails.Also, if I understand correctly, the Cox likelihood would be zero in that case so that the log likelihood is not defined.
For now I have resorted to skipping these problematic batches, but I was thinking that it might be helpful to handle this edge case directly in
CoxPHLoss
. Not sure what's the best way of doing it though.The text was updated successfully, but these errors were encountered: