Question about tied time calculation in Cox loss

[lict99]

torchsurv/src/torchsurv/loss/cox.py

Lines 209 to 215 in a08e9d1

	# sort data by time-to-event or censoring
	time_sorted, idx = torch.sort(time)
	log_hz_sorted = log_hz[idx]
	event_sorted = event[idx]
	time_unique = torch.unique(time_sorted) # time-to-event or censoring without ties
	if len(time_unique) == len(time_sorted):

Hi,

Thank you for this awesome package! 🙌

I'm not a professional statistician, but while using the loss of CoxPH model I came across something that confused me about the calculation of tied times.

As I understand it, tied times occur when two or more events happen at exactly the same time. Based on that, I rewrote a small snippet to identify tied times:

time_sorted, idx = torch.sort(time) 
log_hz_sorted = log_hz[idx] 
event_sorted = event[idx]  # assume event is a bool tensor
time_to_event = time_sorted[event_sorted]

# only consider times with events when calculating tied times
time_to_event_unique = torch.unique(time_to_event)  

if len(time_to_event_unique) == len(time_to_event):
    # Cox log partial likelihood
else:
    # Breslow or Efron

Am I missing something here about how tied times should be detected or handled?

Thanks again for your work on this project!

[tcoroller]

Thank you @lict99 , I have created a PR (#122) and @melodiemonod will review.

[lict99]

After reading some resources, I realized this might not be a big deal from the computational perspective. If there are no real tied times (i.e., two or more events happening at exactly the same time), then both Breslow and Efron methods are equal to the standard Cox no-tie method.

The main reason I'm opening this issue is to point out a possible inconsistency between the package documentation and the actual code behavior.

[lict99]

torchsurv/src/torchsurv/loss/cox.py

Line 32 in e359edb

log_denominator = torch.logcumsumexp(log_hz_flipped, dim=0).flip(0)

By the way, if you plan to switch from using unique times to unique event times, you may also need to adjust the sorting step. Currently, times are sorted in ascending order, but events should also be sorted in descending order (event > censoring) within the same time point.

The reason is that in the Cox no-tie method, the risk set is identified using the cumsum operator. If only sorting by time, then events and censorings that occur at the same time can end up in arbitrary positions relative to each other. This can lead to an incorrect risk set when there are multiple records with the same time but only one is an event (and the others are censorings). This situation should still be considered no-tie, but without ordering event, the risk set construction will be wrong.

[melodiemonod]

Hi @lict99 ,

Thank you for your interest in our package and for raising this point.

Regarding ties in censoring times but not in event times: although it’s possible to introduce additional handling for this case, we prefer not to make changes for the following reasons:

• Our goal is to keep the implementation as lean and minimal as possible.
• When there are ties in censoring times but not in event times, both the Breslow and Efron approximations yield identical results as the standard Cox partial likelihood.

For these reasons, we believe the current implementation is already correct and efficient, and we don’t plan to add extra logic for censoring-time ties at this stage.

[lict99]

Hi @lict99 ,

Thank you for your interest in our package and for raising this point.

Regarding ties in censoring times but not in event times: although it’s possible to introduce additional handling for this case, we prefer not to make changes for the following reasons:

• Our goal is to keep the implementation as lean and minimal as possible.
• When there are ties in censoring times but not in event times, both the Breslow and Efron approximations yield identical results as the standard Cox partial likelihood.

For these reasons, we believe the current implementation is already correct and efficient, and we don’t plan to add extra logic for censoring-time ties at this stage.

Thanks a lot for the clarification! That makes sense.

I would just suggest adding a short note about this in the documentation, since it could help other users (like me😅) who get confused by the difference between documentation and code.

[melodiemonod]

Hi @lict99 , we'll discuss this possible amendment during our next TorchSurv meeting. Thank you again for submitting your issue!