|
| 1 | +# Working on `hyperhyper` |
| 2 | + |
| 3 | +Count-based word embeddings (PPMI + SVD), a reimplementation of Levy, Goldberg & |
| 4 | +Dagan (2015). `CONTRIBUTING.md` covers setup, the test layout and the release |
| 5 | +procedure — read it first. This file covers the things that are **not** obvious |
| 6 | +from the code and that have already been got wrong at least once. |
| 7 | + |
| 8 | +Everything runs under `uv`: `uv run pytest`, `uv run ruff check .`. |
| 9 | + |
| 10 | +## 1. Determinism is a contract, not a nice property |
| 11 | + |
| 12 | +The single most important rule in this repository. Two runs with the same |
| 13 | +arguments must produce the **same bits**, on any machine, at any core count. |
| 14 | +Users key cached artifacts and published results on this. |
| 15 | + |
| 16 | +What that means in practice: |
| 17 | + |
| 18 | +- **`pair_counts.merge_order()` pins the float32 summation order.** float |
| 19 | + addition is not associative, so the order partial matrices are summed in is |
| 20 | + part of the answer. Do not make it depend on completion order, core count or |
| 21 | + chunk count. |
| 22 | +- **Accumulate in float64, narrow once.** The Python loop accumulates into |
| 23 | + `defaultdict(int)` with Python floats (= float64) and only `to_count_matrix` |
| 24 | + casts to float32. Any rewrite that accumulates in float32 rounds at every |
| 25 | + addition instead of once, and the matrix differs in the low bits. |
| 26 | +- **Emit pairs in the loop's order**: centre-major, context position ascending. |
| 27 | + `np.add.at` applies additions in index order, so this is observable. |
| 28 | +- **Never swap `random.Random` for a numpy generator** in the counting path. |
| 29 | + This is the trap this codebase fell into twice *in reasoning* and never in |
| 30 | + code. A numpy RNG would be faster to draw from and would change every |
| 31 | + randomized result at the same seed, permanently. The vectorized counter |
| 32 | + instead **reproduces** the draw stream: one `randint(1, window)` per token in |
| 33 | + token order, one `random()` per subsample-eligible token, interleaved per |
| 34 | + sentence. See `CountPairsClosure._subsample_draws`. |
| 35 | +- **Storage format must not reach the numbers.** The per-chunk RNG is seeded |
| 36 | + from the chunk's `Path(...).stem`. It used to use the full filename, so |
| 37 | + `texts_0.pkl` and `texts_0.npz` drew different numbers — migrating a corpus |
| 38 | + silently changed results without a token moving. |
| 39 | + |
| 40 | +### The gate |
| 41 | + |
| 42 | +`tests/test_pair_counts_equivalence.py` runs the live counter against a **frozen |
| 43 | +snapshot** of the pre-vectorization code (`bench/reference.py`) and requires |
| 44 | +`assert_array_equal` — deliberately not `assert_allclose`, which would wave |
| 45 | +through exactly the drift the gate exists to catch. |
| 46 | + |
| 47 | +Run it before and after any change to `hyperhyper/pair_counts.py`: |
| 48 | + |
| 49 | + uv run pytest tests/test_pair_counts_equivalence.py -m slow |
| 50 | + |
| 51 | +`bench/reference.py` is frozen. Change it only for things that are *not |
| 52 | +counting logic* (how a chunk is read off disk, for instance), and say so in its |
| 53 | +header, which records every such change and why. |
| 54 | + |
| 55 | +If a change cannot hit bit-identity, **that is a finding to raise, not a |
| 56 | +tolerance to widen.** |
| 57 | + |
| 58 | +## 2. Measure before you optimize, and distrust the measurement |
| 59 | + |
| 60 | +Several long-standing beliefs in this repository turned out to be wrong when |
| 61 | +finally measured, and each had a plausible story attached: |
| 62 | + |
| 63 | +- the tokenization process pool was **5.6x slower** than serial, with a table |
| 64 | + proving it sitting directly above the constant that assumed otherwise; |
| 65 | +- `subsample="prob"`/`"dirty"` were documented as "already the fastest |
| 66 | + configurations, not worth vectorizing". They were fastest *per surviving pair* |
| 67 | + and the **slowest per call** — the number a user actually waits on; |
| 68 | +- the README claimed counting was ~8.6% of runtime; it was 28.2%. |
| 69 | + |
| 70 | +So: |
| 71 | + |
| 72 | +- Use the benchmarks: `bench/bench_pair_counts.py` (counting core, deliberately |
| 73 | + single-process — through the pool, spawn startup would hide everything), |
| 74 | + `bench/bench_svd.py` (backends, speed *and* fidelity). |
| 75 | +- **This machine is noisy — ±35% on repeated identical runs.** A single |
| 76 | + before/after pair proves nothing, and back-to-back A-then-B runs drift upward |
| 77 | + and will manufacture a regression that is not there. Alternate the order and |
| 78 | + compare medians. A "15% regression" reproduced three times in one ordering |
| 79 | + disappeared entirely when the ordering was reversed. |
| 80 | +- Quote `best`, not `mean`; report the noise level alongside the number. |
| 81 | + |
| 82 | +## 3. How this repository writes things down |
| 83 | + |
| 84 | +- **Superseded claims are marked, not deleted.** When a measurement overturns a |
| 85 | + documented claim, the old claim stays with a note saying it was wrong and why |
| 86 | + the reasoning was tempting. Several docstrings and CHANGELOG entries are |
| 87 | + structured this way on purpose — the wrong reasoning is the useful part. |
| 88 | +- **Comments say *why*, not *what*.** The density is high and deliberate; |
| 89 | + match it. A comment that restates the line below it is noise here. |
| 90 | +- **Tests carry their own justification.** Tolerances are derived from measured |
| 91 | + noise, not from magic constants, and the test docstrings explain the |
| 92 | + derivation. Vacuity guards exist (`test_delete_oov_actually_changes_the_matrix`) |
| 93 | + because a test that compares a matrix to itself passes happily. |
| 94 | + |
| 95 | +## 4. Boundaries that are easy to cross by accident |
| 96 | + |
| 97 | +- **`tools/` and `bench/` ship in the sdist but not in the wheel.** Maintainer |
| 98 | + scripts (dataset importers, task builders) belong there, never in |
| 99 | + `hyperhyper/`. |
| 100 | +- **Bundled evaluation data is licence-checked per artifact.** `docs/adr/0001` |
| 101 | + records what was verified and where it was read. Do not add a dataset without |
| 102 | + that evidence, and do not infer a dataset's licence from the article that |
| 103 | + describes it — that specific mistake has been made here. |
| 104 | +- **`allow_pickle=False` on every `np.load`.** Bunch directories are a *local |
| 105 | + cache*; the `.npz` files are data, not code. The one remaining pickle path |
| 106 | + (`read_pickle`, for pre-`.npz` chunks) is documented as trusted-input-only. |
| 107 | +- **Corpus chunks: `.npz` is current, `.pkl` is legacy and still read.** The |
| 108 | + format is chosen by extension, never by sniffing. |
| 109 | + |
| 110 | +## 5. Test suite shape |
| 111 | + |
| 112 | +- Default run is the fast suite; `-m slow` adds the full parameter grids. |
| 113 | +- CI runs lint, the fast suite on 3.10–3.13, `test-slow`, and a `floor` job |
| 114 | + against minimum dependency versions. |
| 115 | +- The spaCy-dependent tests skip unless `en_core_web_sm` is installed. That is |
| 116 | + expected locally, not a failure. |
0 commit comments