feat(medcat): Get Stats: harder, better, (probably not) faster, stronger

[adam-sutton-1992]

Hihi,

A new world for get_stats.

Adds three new character metrics:

• Character IoU (which is a pretty standard definition of a metric).
• Golden Character IoU. Which is I believe an interesting metric for us. Essentially the character IoU of all CUIs that have a label. Something of a relation to the recall - I'd say.
• Cohen's Kappa. Used often for measuring inter-annotator agreement. However can be used as a metric of classificiation.

Adds two new "modes":

• "Perfect" linking. Effectively a measure of the performance of the NER step. This doesn't use a perfect linker. It just hacks all labels and predictions to have the same fake cui "NER".
• "Perfect" ner. A measure on the linking step. This uses a the NER component that cheats, and thus is tasked with linking only.
• Obviously the full pipeline is also in there.

Minor changes:

• Returns a StatsCalculator object that can be explored for per cui and overall metrics, along with raw stats.
• Also counts the occasions where no tokens are found for a label span. This results in a false negative, as the model cannot possibly predict that entity. But it also counts the number of these, a decent metric for the tokenizer.
• I've also tested and adapted KFold so the metrics are identical to what they were previously. I've made minimal structural changes during testing and they all passed.
• I hard coded testing for the Stats, and hand calculated what metrics came out should.

I'm a bit unhappy with the naming of the pydantic structure of "RawStats", "ProjectStats", "ModeStats"... It kind of makes sense but is a bit sloppy when reusing it.

[mart-r]

NOTE:
Docs build failures are because of my work on #607 - in order to build there, I need to change the reference to .readthedocs.yaml in settings on RTD side. And this bricks everything other than my PR. I wanted to only change it temporarily (for now) but because there were issues with the docs build I kept it on the PR-specific path for a while. Which is why it failed here.
I should be able to change it back and rebuild the docs with the correct (for this PR) path.

[mart-r]

Overall, I think this is definitely a step in the right direction!
The setup looks good, the output seems easy to use.
And there's still the option to get the same sort of output.

I don't really think we can introduce a breaking change in the manner that you're doing it here due to unkown downstream effects.

So I'd say this thing (i.e the new returned object) needs to be in its own method and the get_stats needs to use this and unwrap the output (like you've done in various bits).

There's a few nagging things.
But also a few things that I think would need to change.
A few structures I'd like to be defined more rigidly (rather than just dict or predefined strings).
A few bits where I feel like we could easily split out the longer methods into smaller ones.
And then there's a matter of documentation in a few places.
And then one place where I asked for a feature for print output stream.

[mart-r]

I think this could use a method that produces the same output as the previous iteration, i.e (to borrow from the other bits of code that already do this):

def to_legacy(self) -> tuple:
    per_cui = (
                full_stats.metrics.per_cui
                if full_stats.metrics is not None
                else {}
            )
    return (
                self.stats.cui_fp,
                self.stats.cui_fn,
                self.stats.cui_tp,
                {cui: metrics.precision for cui, metrics in per_cui.items()},
                {cui: metrics.recall for cui, metrics in per_cui.items()},
                {cui: metrics.f1 for cui, metrics in per_cui.items()},
                self.stats.cui_gold_counts,
                self.stats.examples,
            )

[adam-sutton-1992]

See StatsCalculator.legacy_stats.

[mart-r]

This feels like an Enum?
Right now it's just some magic strings hidden somewhere.

[adam-sutton-1992]

Done!

[mart-r]

This feels a little weird. -1 usually refers to the last element. But here it's "all" but in a list?
And even if you specify a number, you get a list of your requested project as well as all projects.

I feel like this is trying to do too much? I've not gone through all the code so maybe there's a good reason for this, but seems odd to me at this stage.

EDIT:
I think I understand the reasoning here. Because (normally) you're updating a project as well as the aggregate at the same time.
Perhaps this could be renamed to get_project_and_aggregate, return a tuple[ProjectStats, ProjectStats], and remove the defaulting to -1?

[adam-sutton-1992]

yeah done. Just split it out to:

    def get_project_stats(self, project_index: int) -> ProjectStats:

    def get_aggregate_stats(self) -> ProjectStats:

[mart-r]

Would be great to have this format defined somewhere, e.g a TypedDict.

[adam-sutton-1992]

Sorry not sure exactly what needs a typed dict? the annotations or the valid_cuis?

[mart-r]

The format of the contents of gold_anns.
I.e

class GoldenAnnotation(TypedDict):
    start: int
    end: int
    cuis: list[str]
    cui: str
    text: str
    raw: str

Don't need to use the constructor for it, but if you type the output, the type checkers (like mypy) can check that the stuff being accessed actually exists.

I.e have the return type here list[GoldenAnnotation] so everything down the road knows the format.

[mart-r]

Perhaps we can split this up as well?
I.e have this iterate over the projects, call another method for the preparation, and then set the metrics.
Something like:

def _prepare_metricS(self, *args):
    # do the work
    return overall, per_cui
def compute_metrics(self, *args):
    for project_stats in self.stats.get_projects(project_index):
        overall, per_cui = self._prepare_metrics()
        mode_stats.metrics = Metrics(
                overall=overall,
                per_cui={
                    cui: CUIMetrics(**metrics)
                    for cui, metrics in per_cui.items()
                },
            )

[adam-sutton-1992]

Done

[mart-r]

Since we're redoing this entire thing thoroughly, perhaps allow (and use) a stream in the method signature here?
That way we'd make it easier to capture the information (either in code or in specific files for logging).

I'd just say allow for a stream: SupportsWrite[str] | None = None keyword argument and pass that on to print(*, file=stream).

[adam-sutton-1992]

Done.

[mart-r]

This is a breaking change in terms of the return type.

The problem is that we don't know whether or what is using our software somewhere downstream.
And as such, I'd be extremely reluctant in making a drastic change like this here. You can see the effects in the fact that the tutorials initially failed and needed to be patched.

For reference, this might break something UCLH folks are doing with the MiADE (recently updated (or mid update) to v2) or CogStack ModelServe (not sure whether they've full updated or which version of medcat they're using in production, but I know they did do a v2 update and were using stuff like get_stats).

I would prefer that the old signature remain (at least for now). I.e you'd unwrap the output like you've done in the tutorials or in kfold stats.
And this new stuff would be in another method, get_stats_new, get_stats2, or something like that.

[adam-sutton-1992]

I've wrapped get_stats around get_stats_calculator. Get Stats calculator will return the entire object. Get Stats will do as previous.

[mart-r]

Perhaps stuff below here (and above do_print) should be a method on StatsCalculator? I.e StatsCalculator.compute_all_metrics
It should already have the necessary options. And would make this method a little neater.

[adam-sutton-1992]

Done!