stats: longest-prefix factor attribution; a colon inside a level is not an interaction - #37
Open
ChrisW09 wants to merge 1 commit into
Open
stats: longest-prefix factor attribution; a colon inside a level is not an interaction#37ChrisW09 wants to merge 1 commit into
ChrisW09 wants to merge 1 commit into
Conversation
…rsing
- factor attribution used the FIRST factor whose name prefixes the
coefficient term, so with factors like ['retrieve', 'retrieve.top_k']
the term 'retrieve.top_k2' was attributed to 'retrieve' — reporting
the wrong factor in significant_factors while the label (which already
used longest-prefix) said retrieve.top_k=2. Now longest prefix wins,
via a shared _term_factor helper.
- every ':' in a term was treated as R's interaction separator, but a
':' can appear inside a level (model='llama3:8b' -> coefficient
'modelllama3:8b'). Main effects were flagged as interactions, labels
garbled to 'model=llama3 × 8b', and the statsmodels path produced raw
patsy junk with factor=None. R terms now split on ':' only when every
piece starts with a known factor (_split_r_term); statsmodels terms
are parsed by matching the C(Q('...'))[T....] groups directly instead
of pre-splitting.
Fixes #7
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7.
Bug 1 — first-prefix attribution.
ordinal.pyandlogistic.pyattributed a coefficient tonext((f for f in factors if term.startswith(f)), None). With factors["retrieve", "retrieve.top_k"](the shape the codebase's owntest_readable_clmm_termtreats as real), the R termretrieve.top_k2was attributed toretrieve— sosignificant_factorsreported the wrong factor while the row's label saidretrieve.top_k=2. Both files now use a shared_term_factor(longest prefix), matching_readable_term's existing rule.Bug 2 — colon inside a level. Every
:was treated as R's interaction separator, but ollama-style levels (model="llama3:8b") put colons inside the coefficient name (modelllama3:8b). Verified effects: main effects flaggedinteraction=True, labels garbled tomodel=llama3 × 8b, and the statsmodels path emitted raw patsy junk withfactor=None. Fixes:_split_r_termtreats:as a separator only when every piece starts with a known factor name (a heuristic — the robust long-term fix is for clmm.R/glmm.R to emit structured per-coefficient factor+level, noted in the docstring)._parse_statsmodels_termnow matches theC(Q(…))[T.…]groups withfinditerinstead of pre-splitting on:, so a colon inside[T.llama3:8b]is naturally part of the level.Regression tests cover longest-prefix attribution, both split behaviors, readable labels, and the statsmodels main-effect/interaction cases. The existing
test_readable_clmm_termstill passes unchanged.Test: full cafe-core suite — 110 passed (1 pre-existing failure = #4, fix in PR #34).
🤖 Generated with Claude Code