Skip to content

Retire binary_compare(), which reports every below-threshold pair as a false positive #246

Description

@sromoam

Summary

BaseComparator.binary_compare() returns (0, 1) labelled "false positive" for
any below-threshold pair. By Stickler's own vocabulary that case is a false
discovery
, and FD is only half of FP. Retire the method in 0.8.0 alongside the
other two scheduled removals (#215, #226).

The problem

src/stickler/comparators/base.py:137:

def binary_compare(self, str1, str2) -> Tuple[int, int]:
    score = self.compare(str1, str2)
    if score >= self.threshold:
        return (1, 0)  # True positive
    else:
        return (0, 1)  # False positive

A confusion-matrix cell is fixed by two independent axes: whether something was
predicted, and whether something was actually there. A comparator only ever sees
two values that both exist, since the shared None policy resolves absent values
before dispatch. So the only situation this method can be looking at is both
sides present, similarity below threshold
.

The docs already name that case, at
docs/docs/Getting-Started/thresholds-and-metrics.md:138:

| False Discovery | FD | Both non-null but similarity < threshold | Wrong prediction |

and define the relationship two lines later:

FP = FA + FD

So (0, 1) is labelled with the umbrella term when the only reachable case is
the FD sub-category. FA (an unmatched prediction) can never occur here, because
an unmatched prediction has nothing to compare against and never reaches a
comparator. FN and TN are unreachable for the same reason. A two-tuple is
presented as a confusion-matrix reading when three of the five categories cannot
be expressed in it.

How this bites someone

Build per-field counts by looping binary_compare and summing columns, then
compute precision = tp / (tp + fp). The fp column holds false discoveries
only and silently omits every false alarm, because extra predicted items were
never compared. Precision comes out inflated exactly when the model
over-predicts
, which is the failure mode you most want to catch. Nothing
errors and the number looks plausible.

Two facts that make this cheap, and worth more than its priority suggests

Zero callers in src/. Only five test files reference it. Nothing in the
comparison engine, the confusion-matrix layer, or the evaluators uses it.

It is the sole consumer of the comparator-level threshold. base.py:154 is
the only place self.threshold is compared against anything in the entire
comparator layer; every other match is __repr__ string interpolation.

That second point is the interesting one. This method is the whole reason
"comparator threshold" exists as a concept, and it is threshold #1 of the four
that thresholds-and-metrics.md then has to explain is ignored during
evaluation
(lines 23 and 96). External feedback called threshold proliferation
"the single most confusing thing we hit while learning the library." Retiring
binary_compare lets threshold #1 leave the mental model entirely, so this is a
simplification of the documented threshold story rather than cosmetic cleanup.

Proposed change

  1. Emit a DeprecationWarning from binary_compare() naming the confusion:
    the second element is an FD, not an FP, and callers wanting real
    confusion-matrix counts should use compare_with(include_confusion_matrix=True).
  2. Remove in 0.8.0, batched with Remove the compare() -> _compare() deprecation shim #215 and Remove the deprecated ComparableField(aggregate=) parameter and its dead code path #226.
  3. Update the five test files to assert the deprecation rather than the
    behaviour.
  4. Once removed, drop threshold Pushing an initial commit. #1 from thresholds-and-metrics.md and reduce
    "The Four Thresholds" to three.

Explicitly out of scope: the threshold= parameter on BaseComparator and
its subclasses. That is a public constructor signature and may appear in
serialized comparator configs, so it needs its own investigation. This issue
retires the method only.

Also, a small docs bug found alongside

docs/docs/Getting-Started/Contributing/code-style.md:85 shows the signature as
-> float; it returns Tuple[int, int]. It is a naming-convention example so
nobody is copying it for behaviour, but it should be corrected.

Context

Raised as item 2.2 (P3) in the "Feedback Doc on Existing Stickler State"
(2026-07-08), audited against 0.7.0. It was the only item in that document with
no issue tracking it.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    Status
    Backlog

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions