googlefonts / googlefonts/fontc
ttx_diff: per-font similarity uses character count instead of line count
- 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.