googlefonts / googlefonts/fontc

ttx_diff: per-font similarity uses character count instead of line count

Open
#1,899 0 comments 0 reactions 1 assignee Claimed by @cmyr View on GitHub
Dominant language
Rust
Stars
193
Forks
21
Avg merge
1d 17h
Merged PRs (30d)
65

Description

In `ttx_diff/src/ttx_diff/core.py`, the [`jsonify_output`](https://github.com/googlefonts/fontc/blob/3e48cbb328689f12ecd426ab5cdc7527e808da29/ttx_diff/src/ttx_diff/core.py#L1174-L1211) function computes a per-font similarity score using variables named `same_lines` / `different_lines` / `n_lines`. However, the values in the `fontc`/`fontmake` dicts are _strings_ (XML text per table, returned by [`extract_comparables`](https://github.com/googlefonts/fontc/blob/3e48cbb328689f12ecd426ab5cdc7527e808da29/ttx_diff/src/ttx_diff/core.py#L1221) with return type `dict[str, str]`), so `len(s)` is the **character count**, not the line count (lines 1194-1196).

Meanwhile, [`diff_ratio()`]( https://github.com/googlefonts/fontc/blob/3e48cbb328689f12ecd426ab5cdc7527e808da29/ttx_diff/src/ttx_diff/core.py#L1234-L1238) computes a _line-level_ ratio via `SequenceMatcher.quick_ratio()`[^1]: the input strings are split via `splitlines()`, and each complete line is treated as an atomic element by the `SequenceMatcher`, so the returned ratio reflects "the fraction of lines that are the same" (says a comment).

But that line-level ratio is then multiplied by the character count:

```python
n_lines = max(len(s1), len(s2)) # characters, despite the name
same_lines += int(n_lines * ratio) # line-ratio x char-count
different_lines += int(n_lines * (1 - ratio))
```

This means the per-font "total" similarity score is effectively **character-weighted**: tables with longer XML lines get proportionally more weight than tables with many short lines, even at the same line-level similarity ratio. The practical impact is likely small since TTX line lengths should be reasonably consistent, but the variable names (`same_lines`, `different_lines`, `n_lines`) suggest the intent was line-level weighting.

I wonder if this was intentional? If not, the fix would be to use `len(s.splitlines())` instead of `len(s)` for the weighting, and rename variables accordingly.

[^1]: Note: [`quick_ratio()`](https://github.com/python/cpython/blob/171e0facc4131f74baec38f58fb0971c52ac2c8a/Lib/difflib.py#L623-L650) is used rather than `ratio()`, so the result is a multiset-based upper bound that ignores line ordering. This is probably fine for TTX XML since table structures should be consistently ordered by both compilers, though worth knowing.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.