From d5876a46d382df16bd35b1fbf8edda8b26a6b09d Mon Sep 17 00:00:00 2001 From: wassname <1103714+wassname@users.noreply.github.com> Date: Tue, 23 Jun 2026 21:25:33 +0800 Subject: [PATCH] correctness fixes from gpt-5.5 review (ordinal path + fail-fast asserts) Second external review (gpt-5.5, correctness-focused) on the post-cleanup tree. Found no off-by-one/double-flip in the ordinal canonicalization+keying. Fixed the parts I agreed with and could verify on the path the experiment uses: - read.py: NaN-safe answer-token renorm. p_a/pmass poisons the profile with NaN when pmass underflows to 0 at coherence collapse -- exactly when pmass should just flag it. softmax(logp_allowed) is identical when pmass>0 and stable at collapse. - maps.ipsative_pca: move SVD sign-stabilization INTO the helper so it and plot_ipsative_pca share one orientation (saved coords could otherwise mirror the figure). - instrument: assert ordinal answer_space is ['1'..scale_max] IN ORDER (reduce_ordinal weights by position; a reordered space silently inverts E) -- was length-only. - instrument.per_item_categorical: assert per-item dimension/sign agree across frames and frames are distinct, instead of silently averaging under rows[0]'s metadata. - pyproject: move matplotlib+textalloc to an optional `maps` extra; evals stay headless. - tests: drop imports of the deleted reduce_nominal/expected_value, inline the expectation, remove the now-impossible nominal-reducer test. Deferred (flagged to maintainer): two NaN/window issues in guided.py's forced-choice rollout (nominal evaluate() path) -- not exercised by this experiment, can't smoke-test, and the NaN-as-collapse-signal there is a deliberate design. Verified: experiment smoke green on all 4 instruments (no assert false-fires), 6 pure unit tests pass, headless import clean, 16pf map renders. Co-Authored-By: Claudypoo <288921227+claudypoo@users.noreply.github.com> --- pyproject.toml | 10 ++++++++-- src/tinymfv/instrument.py | 10 +++++++++- src/tinymfv/maps.py | 17 ++++++++++------- src/tinymfv/read.py | 9 ++++++--- tests/test_instrument.py | 23 +++++++++-------------- 5 files changed, 42 insertions(+), 27 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 514a120..e5731f5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -17,8 +17,14 @@ dependencies = [ "tabulate", "datasets", "numpy", - "matplotlib>=3.8", # tinymfv.maps: culture-map + range viz - "textalloc>=1.2.3", # non-overlapping label placement on the maps +] + +[project.optional-dependencies] +# tinymfv.maps (culture-map + range viz) only; `import tinymfv` and the evals stay headless, +# numeric-only consumers (steering-lite) skip this. Install with `pip install tiny-mfv[maps]`. +maps = [ + "matplotlib>=3.8", + "textalloc>=1.2.3", # non-overlapping label placement on the maps ] [dependency-groups] diff --git a/src/tinymfv/instrument.py b/src/tinymfv/instrument.py index dd9d630..7b4b828 100644 --- a/src/tinymfv/instrument.py +++ b/src/tinymfv/instrument.py @@ -73,7 +73,10 @@ class Instrument: def __post_init__(self): if self.kind == "ordinal": - assert len(self.answer_space) == self.scale_max, "ordinal answer_space must be 1..scale_max" + assert self.answer_space == [str(i) for i in range(1, self.scale_max + 1)], ( + f"ordinal answer_space must be ['1'..'{self.scale_max}'] IN ORDER -- reduce_ordinal " + f"weights by position (w = 1..scale_max), so a reordered space silently inverts E; " + f"got {self.answer_space}") if self.kind == "nominal" and self.answer_to_dim is None: self.answer_to_dim = {a: a for a in self.answer_space} # Cross-scale caveat: a 1-7 human histogram (HSQ) cannot share a 5-way soft-NLL/JS @@ -125,6 +128,11 @@ def per_item_categorical(per_row: list[dict], kind: Kind) -> dict[str, dict]: assert len(frame_counts) == 1, f"heterogeneous frame counts per item: {frame_counts}" out: dict[str, dict] = {} for iid, rows in by_id.items(): + # Per-item rows differ only by frame; dimension + sign must agree, frames must be distinct. + # Else we would silently average incompatible distributions under rows[0]'s metadata. + assert len({r.get("dimension") for r in rows}) == 1, f"{iid}: inconsistent dimension across frames" + assert len({r.get("sign", 1) for r in rows}) == 1, f"{iid}: inconsistent sign across frames" + assert len({r["frame"] for r in rows}) == len(rows), f"{iid}: duplicate frame in rows" canon = [canonicalize_to_forward(r["p"], r["frame"], kind) for r in rows] C = np.stack(canon) spread = float(max((np.abs(C[i] - C[j]).sum() diff --git a/src/tinymfv/maps.py b/src/tinymfv/maps.py index ac208bc..436711f 100644 --- a/src/tinymfv/maps.py +++ b/src/tinymfv/maps.py @@ -55,12 +55,19 @@ def row_centre_op(K: int) -> np.ndarray: def ipsative_pca(M: np.ndarray, k: int = 2): """Row-centre each row of M (societies x K), then PCA across rows. - Returns (P, Vt, var, mu, Pc); project a new point v via ((v @ Pc) - mu) @ Vt[:k].T.""" - Pc = row_centre_op(M.shape[1]) + Returns (P, Vt, var, mu, Pc); project a new point v via ((v @ Pc) - mu) @ Vt[:k].T. + SVD signs are stabilized here (PC1 loads + on factor 0, PC2 on factor 1) so this helper and + any plot built on it share ONE orientation -- otherwise saved coords could mirror the figure.""" + K = M.shape[1] + Pc = row_centre_op(K) Mp = M @ Pc mu = Mp.mean(axis=0) Mc = Mp - mu _, S, Vt = np.linalg.svd(Mc, full_matrices=False) + if Vt[0, 0] < 0: + Vt[0] = -Vt[0] + if Vt.shape[0] > 1 and Vt[1, 1 % K] < 0: + Vt[1] = -Vt[1] var = (S ** 2) / (S ** 2).sum() return Mc @ Vt[:k].T, Vt, var, mu, Pc @@ -100,11 +107,7 @@ def plot_ipsative_pca(instr: Instrument, dims: list[str], countries: list[str], import textalloc as ta except ImportError: ta = None - P, Vt, var, mu, Pc = ipsative_pca(M) - if Vt[0, 0] < 0: # stabilise SVD sign: factor[0] loads +PC1 - Vt[0] = -Vt[0] - if Vt[1, 1 % len(dims)] < 0: - Vt[1] = -Vt[1] + P, Vt, var, mu, Pc = ipsative_pca(M) # signs already stabilized inside the helper P = (M @ Pc - mu) @ Vt[:2].T def proj(v): diff --git a/src/tinymfv/read.py b/src/tinymfv/read.py index 2669f0c..8f6445e 100644 --- a/src/tinymfv/read.py +++ b/src/tinymfv/read.py @@ -72,9 +72,12 @@ def read_items(model, tok, instr: Instrument, items: list[InstrItem], answer_ids enc = tok(texts, return_tensors="pt", padding=True, add_special_tokens=False).to(device) logits = model(**enc).logits[:, -1, :].float() # [B, V] next-token logp = F.log_softmax(logits, dim=-1) - p_a = logp[:, gid].exp() # [B, A] prob on each answer token - pmass = p_a.sum(dim=-1) # [B] - p_norm = p_a / pmass[:, None] # [B, A] within allowed + logp_a = logp[:, gid] # [B, A] logprob on each answer token + pmass = logp_a.exp().sum(dim=-1) # [B] coherence check: mass on allowed tokens + # softmax over the allowed logprobs == p_a / pmass when pmass > 0, but NaN-safe: at coherence + # collapse pmass underflows to 0 and the divide would poison the whole profile with NaN; the + # softmax still returns a valid within-allowed distribution and pmass separately flags the drop. + p_norm = F.softmax(logp_a, dim=-1) # [B, A] within allowed for j, it in enumerate(chunk): out.append({ "id": it.id, "frame": it.frame, diff --git a/tests/test_instrument.py b/tests/test_instrument.py index 4a5359b..df1904d 100644 --- a/tests/test_instrument.py +++ b/tests/test_instrument.py @@ -8,13 +8,17 @@ import numpy as np from tinymfv.instrument import ( Instrument, InstrItem, canonicalize_to_forward, per_item_categorical, - reduce_nominal, reduce_ordinal, expected_value, shuffle_dimensions, + reduce_ordinal, shuffle_dimensions, ) M = 5 ONEHOT = {d: np.eye(M)[d - 1] for d in range(1, M + 1)} # ONEHOT[4] = mass on scale point 4 +def _E(p, scale_max): # expected scale point; reduce_ordinal computes this inline now + return float((np.asarray(p) * np.arange(1, scale_max + 1)).sum()) + + def _ord_instr(items): return Instrument("t", "endorsement", "ordinal", ["1", "2", "3", "4", "5"], ["care", "authority"], items, prefill="(") @@ -33,7 +37,7 @@ def test_forward_expectation(): rows = [{"id": "1", "frame": "forward", "p": ONEHOT[4], "pmass_allowed": 1.0, "dimension": "care", "sign": 1, "human_label": None}] items = per_item_categorical(rows, "ordinal") - assert abs(expected_value(items["1"]["p"], M) - 4.0) < 1e-9 + assert abs(_E(items["1"]["p"], M) - 4.0) < 1e-9 prof = reduce_ordinal(items, _ord_instr([])) assert abs(prof[0] - 4.0) < 1e-9 # care assert np.isnan(prof[1]) # authority has no items @@ -45,8 +49,8 @@ def test_frame_consistency(): "dimension": "care", "sign": 1, "human_label": None}] inv = [{"id": "1", "frame": "inverted", "p": ONEHOT[5], "pmass_allowed": 1.0, "dimension": "care", "sign": 1, "human_label": None}] - e_fwd = expected_value(per_item_categorical(fwd, "ordinal")["1"]["p"], M) - e_inv = expected_value(per_item_categorical(inv, "ordinal")["1"]["p"], M) + e_fwd = _E(per_item_categorical(fwd, "ordinal")["1"]["p"], M) + e_inv = _E(per_item_categorical(inv, "ordinal")["1"]["p"], M) assert abs(e_fwd - 1.0) < 1e-9 and abs(e_inv - 1.0) < 1e-9 # canonicalization makes them agree @@ -59,7 +63,7 @@ def test_keying_is_not_double_flip(): rows = [{"id": "x", "frame": "inverted", "p": ONEHOT[5], "pmass_allowed": 1.0, "dimension": "care", "sign": -1, "human_label": None}] items = per_item_categorical(rows, "ordinal") - assert abs(expected_value(items["x"]["p"], M) - 1.0) < 1e-9 # canonical agreement-with-item = 1 + assert abs(_E(items["x"]["p"], M) - 1.0) < 1e-9 # canonical agreement-with-item = 1 prof = reduce_ordinal(items, _ord_instr([])) assert abs(prof[0] - 5.0) < 1e-9 # keyed factor score = 5, not 1 (no double flip) @@ -81,15 +85,6 @@ def test_frame_spread_diagnostic(): assert abs(items["1"]["frame_spread"] - 2.0) < 1e-9 -def test_reduce_nominal(): - instr = Instrument("mfv", "salience", "nominal", ["care", "authority"], - ["care", "authority"], [InstrItem("1", "v1")], prefill="(") - rows = [{"id": "1", "frame": "forward", "p": np.array([0.8, 0.2]), "pmass_allowed": 1.0}, - {"id": "2", "frame": "forward", "p": np.array([0.4, 0.6]), "pmass_allowed": 1.0}] - prof = reduce_nominal(per_item_categorical(rows, "nominal"), instr) - assert np.allclose(prof, [0.6, 0.4]) # mean choice frequency - - def test_negative_control_shuffle(): rng = np.random.default_rng(0) rows = [{"id": str(i), "frame": "forward", "p": ONEHOT[(i % 5) + 1], "pmass_allowed": 1.0,