nci / scores

Metrics for the verification, evaluation and optimisation of forecasts, predictions or models.
https://scores.readthedocs.io/
Apache License 2.0
51 stars 15 forks source link

Location of isotonic fit #247

Closed tennlee closed 4 months ago

tennlee commented 4 months ago

The isotonic_fit function is currently included in three modules - processing (root implementation), continuous and probability.

I accepted this as part of the large merge request, where it was mentioned on the way through. I thought it wasn't worth holding up the MR for this discussion, but I wanted to re-raise it for further consideration.

If isotonic fit is not a score per se, then even thought it can be used on both continuous and probablistic data, I'm not sure it needs to be in all three places. I would be inclined to consider removing it from continuous and probability, but I could be convinced. It's not doing much harm having it in all three places, but it might be more parsimonious to just have it in the one place.

For things that are scores, I think having them in both places makes sense, but I'm less convinced when it's data processing. I'm not proposing we need to do anything right now, but ideally we should come to a view before version 1, as it will become problematic to remove anything from an API going forward from that point.

nicholasloveday commented 4 months ago

While it's not a score, it tells us about the calibration of a forecast as it is the recommended method for producing reliability diagrams. This is similar to how we put additive and multiplicative bias under scores.continuous

I think PIT (https://github.com/nci/scores/issues/142) will be added to probability too. It's not a score but a measure of calibration just like isotonic regression.

I think those categories are designed group the metrics and tools into the form of the forecast as we haven't strictly been limiting them to scoring functions.

If you want to cut down the amount of places that it can be used, then I suggest we remove it from scores.processing and leave it in the other two.

tennlee commented 4 months ago

That's a reasoned approach. Let's just leave it as-is then. The modules just needs to make sense. The main thing is that you are aware this will be hard to reverse later, and you have noted that. Thanks.