thanks @mibaumgartner, this looks good. My understanding is that the PartialMetric interface is very useful for aggregating metrics from multiple batches/subjects/patients. Perhaps the class name could be CumulativeMetrics or AggregateMetrics?

I agree that we include ROCAUC and Dice. But the class name Dice is used to indicate single batch output. I think we need another class name for this cumulative Dice...

we could leave the intermediate caching for future PRs. My experience is that It'll reduce the code readability a lot. We'll consider multiple types of metrics sharing the same intermediate cache iff we identify serious performance bottlenecks in metrics computation -- we may run into these cases, for example, computing different types of scores where all scores are based on the same connected component analysis outcome. In this case we don't want to compute CCA multiple times

@wyli thank you for your quick feedback. CumulativeMetrics is a much better name 👍
I'll add the changes tomorrow :)

The overall idea is great.
As @myron already updated the MeanDice metric with a class API, could you please help update your PR to unify all the things?

Thanks.

The general functionality should be complete now. The points I changed:

• update to master
• CumulativeMetric now extends Metric by the ability to add samples. The final computation is still a just a call.
• Renamed DiceMetric to Dice
• extracted a subfunction from compute_dice to avoid code duplication

A few open points:

• Was the renaming from DiceMetric to Dice okey?
• Should cumulative metrics get individual samples or batches? (the current implementation assumes batches and the method should potentially be renamed to add_batch)
• How should the device of the intermediate representation be handled for cumulative metrics? Should we introduce a parameter to decide this in the init?
  (e.g. device=None, does not touch the device, device='cpu' automatically pushed the intermediate values to the cpu to avoid GPU mem leaks)
• Probably more important for future PRs: What should the metrics return? (float, tensor, numpy)

I'll update/add the docstrings and unittests tomorrow after we finalized the design :)

Thanks! I think the renaming looks good, and yes we need a device argument for the cumulative metrics (defaults to be the same as the input samples). they should return tensor for now, probably need another PR to make ROCAUC a tensor implementation

for the CumulativeMetric I'm also thinking about the use case of, for example, during each batch training/inference iterations, compute Dice score for each item in the batch, and after k iterations/epochs, aggregate the scores across the batches by computing mean/median/95 percentile...
Do we implement this case using the proposed API or is it out of the scope of this PR?

At first I thought this would probably out of scope of this PR but after thinking about it a little bit, there might be a fairly simple solution which would fit pretty well in here.
The aggregation of batch specific results is pretty abstract so it could probably be shared by many metrics. I think the best solution would be to create a wrapper by subclassing CumulativeMetric which can take any other metric and aggregate its results over multiple batches. Could look something like this:

class AggregatedMetric(CumulativeMetric):
    def __init__(per_batch_metric, aggregation_mode, device):
        ...
   
    def __call__(self):
        # return aggregated metrics

    def add_sample(self, ..):
        # compute per_batch_metric and save it for later aggregation

Edit: Not sure if this is the most user friendly approach though 🙈

after looking at your code I think another slightly different option is to isolate the intermediate caching interface:

class ResultBuffer(ABC):
    def update(self, ...):
        # update the buffer with a new sample
        # this could support multi-thread/process context
        # e.g. inference on multiple samples in parallel
    def get_samples(self, ...):
        # read out the buffer or part of the buffer
        # one implementation would be:
        #     return torch.cat(self.y_pred_stored), torch.cat(self.y_stored)
        # or we could get the last 10 samples if needed:
        #     return torch.cat(self.y_pred_stored[-10:]), torch.cat(self.y_stored[-10:])
    def reset(self):
        # reset the buffer

then the Dice implementations would be:

class CumulativeDice(ResultBuffer, Metric):

class AggregateDice(ResultBuffer, Metric):

then in the near future, we could have fast aggregation Dice, something like:

# start a loader for pairs of groundtruth and prediction
pairwise_dataloader = torch.DataLoader(num_workers=5, ...)

agg_dice = AggregateDice()
with multiprocess.pool as p:  # this won't work in practice, just want to show the idea
    assert isinstance(agg_dice, ResultBuffer)
    p.map(agg_dice.update, [(g, t) for g, t in pairwise_dataloader]) 
agg_dice()  # get the final aggreated result

thanks for the discussion here, this will be closed in favour of #2314