feat(medcat): Get Stats: harder, better, (probably not) faster, stronger by adam-sutton-1992 · Pull Request #606 · CogStack/cogstack-nlp

adam-sutton-1992 · 2026-08-18T20:45:32Z

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 · 2026-08-19T10:47:50Z

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 · 2026-08-20T13:45:54Z

+    ner: ModeStats | None = None
+    linking: ModeStats | None = None
+
+    _MODE_FIELDS = {


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

Do you still need the mapping here? Can't you just use the .value of the enum?

mart-r · 2026-08-20T13:49:13Z

+    )
+
+
+    def get_projects(self, project_index: int = -1) -> list[ProjectStats]:


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?

yeah done. Just split it out to:

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

mart-r · 2026-08-20T15:20:07Z

+    def _safe_mean(self, values):
+        return sum(values) / len(values) if values else 0.0
+
+    def compute_metrics(


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() }, )

mart-r · 2026-08-20T15:28:50Z

+                  ner_performance: bool = False,
+                  linking_performance: bool = False,
+                  extra_cui_filter: Optional[set[str]] = None,
+                  do_print: bool = True,) -> "StatsCalculator":


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.

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

A few doc strings I'd like to see, plus removal of added comments from test_kfold.py.

And the project + aggregate issue that's still present. Would be nice to clean that up, but I don't think it's high priority. Can easily leave as is.

The rest is more or less just nagging.

mart-r · 2026-08-25T13:40:51Z


    def setUp(self) -> None:
        super().setUp()
+        # return (self.fps, self.fns, self.tps,


Maybe we don't need the comments to be added here? Probably left over when you had the breaking change?

mart-r · 2026-08-25T13:45:50Z

+        """Count gold annotations for a project and all-projects aggregate."""
+        project_stats = self.stats.get_project_stats(project_index)
+        aggregate_stats = self.stats.get_aggregate_stats()
+        for project_stats in (project_stats, aggregate_stats):


Supernag: the shadowing of project_stats (that is originally the specific project's stats rather than here being either that or the aggregate) is not ideal.

mart-r · 2026-08-25T13:48:47Z

+class StatsCalculator:
+    """Calculates statistics for entity linking."""
+
+    BUCKET_FULL = MetricMode.FULL


NAG: Do we need to reference them here ? Can't we just use MetricMode.NER and/or MetricMode.LINKING directly?

mart-r · 2026-08-25T14:16:12Z

+                           filter_fp_by_cui: bool = True) -> None:
+        # Track which predictions have been matched
+        matched_preds: set[int] = set()
+        aggregate_stats = self.stats.get_aggregate_stats()


NAG[Proj+Aggr]: I find it annoying that you're having to get both the project-specific stats and the aggregate states and work on them separately. I feel like it's bound to lead us to a situation where only one is changed by accident.

Perhaps using a list here (project_and_aggregate = [all_projects_state, project_state]) and iterating over this when updating?
Or using a composite that updates both? Though this would be more work + code to maintain.

mart-r · 2026-08-25T14:16:55Z

+
+    def _update_project_stats(
+        self,
+        project_state: ModeStats,


NAG[Proj+Aggr]: Same here, perhaps a list then?

mart-r · 2026-08-25T14:17:48Z

+        # cui_cohen_k[CUI] = sum of per-document CUI-specific Kappa
+        #                 -> divide by number of documents where the CUI is evaluated
+        """
+        aggregate_stats = self.stats.get_aggregate_stats()


NAG[Proj+Aggr]: I guess this is where it starts.

mart-r · 2026-08-25T14:21:02Z

-                 extra_cui_filter: Optional[set[str]] = None
-                 ) -> 'StatsBuilder':
-        """Get the stats builder from a model pack and some extra information.
+def get_stats_calculator(cat: CAT, 


This could use a doc string.
I'd like to include why this might be better (i.e how you get granular access to the data), but also include the fact that the calculator has already done its job (the calculations) and there is no need to call an extra method to do that or something like that.

mart-r · 2026-08-25T14:21:41Z

+        calculator.print_stats(epoch, to_print)
+    return calculator
+
+def get_stats(cat: CAT, 


This also does still need a doc string. Doesn't need to be the same as it had before, but something at least.

adam-sutton-1992 added 6 commits August 12, 2026 17:55

initial commit

05d3487

finished testing, fixes, linting, and mypy

7c68072

adapted kfold, tested kfold

3fffead

fixed linting

8133738

utils mypy fix

f81d1eb

linting...

378da76

adam-sutton-1992 added 3 commits August 19, 2026 13:52

fixed tutorial

1451efe

fixes to training utils

9e2ed60

notebook fix

70ce00f

mart-r requested changes Aug 20, 2026

View reviewed changes

adam-sutton-1992 added 5 commits August 24, 2026 20:13

changes as per suggestions, along with fixes to mypy and testing

77a35ca

spacing

ed0bf34

restore notebook from previous version

330dfe6

fixed testing utils

25ad0c1

enum corrections

3d04c65

mart-r requested changes Aug 25, 2026

View reviewed changes

		)


		def get_projects(self, project_index: int = -1) -> list[ProjectStats]:

Uh oh!

Conversation

adam-sutton-1992 commented Aug 18, 2026

Uh oh!

mart-r commented Aug 19, 2026

Uh oh!

mart-r left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mart-r left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants