docs(contract): enforce the corrected manuscript equations; retire the erratum - #42
Merged
Conversation
…e erratum The manuscript's Entropy section has been corrected: eqs 3-4 now weight by the CONDITIONAL (matching the log) and eq 4's left-hand side reads H(p(phi|c)) instead of the impossible H(p(c)). Both now agree with the implementation exactly. Verified before changing anything (small asymmetric 3x2 joint): eq 3 1.251629 / 1.061278 == _clonotypic_one MATCH eq 4 0.721928 / 1.0 / 0.591673 == _phenotypic_one MATCH eq 5 0.288703 == _mi_from_joint MATCH Added tests that transcribe eqs 2-6 LITERALLY from the manuscript and assert the code computes exactly that. These are now the primary definitional tests; the pre-existing identity tests (uniform -> log2 k, degenerate -> 0, zero-mass -> NaN, support-only, the MI decomposition) stay, but they pin CONSEQUENCES of a formula rather than the formula itself. SOURCE_ERRATA removed from the manifest, __all__, the two note_eq references, the module docstring, METRICS_CONTRACT.md and CLAUDE.md. A short historical note stays in the prose so a reader who finds the old revision is not confused. test_note_literal_formula_would_break_the_decomposition is REFRAMED rather than deleted, as test_marginal_weighting_is_not_an_entropy: marginal-weighting is the natural way to mis-transcribe these equations and it fails two ways at once -- the value can exceed log2(|C|) (impossible for an entropy over |C| outcomes) and it makes the MI decomposition return a NEGATIVE mutual information. Worth keeping as a standing guard independent of any manuscript revision. ONE LIVE DEVIATION, now explicit. Eq 6 defines NMI = I / ((1/2)(H(c)+H(phi))) -- the MEAN denominator -- while tcri defaults to 'min'. On the contract's test joint that is 0.239 (eq 6) vs 0.293 (default), ~23% apart. 'min' is deliberate: the mean denominator scales with log2(C) and so is not comparable across groups with different clone counts. Recorded as SANCTIONED_EXTENSIONS['normalize_mode_default'] and pinned by test_eq6_nmi_is_the_average_denominator, which asserts BOTH that 'average' reproduces eq 6 AND that the default does not -- so the divergence cannot go silent. Anything reproducing the note's benchmark must pass normalize_mode='average'. Metrics contract 12 -> 17 tests. Full suite 169 passed, 3 skipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
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.
The manuscript's Entropy section has been corrected — eqs 3–4 now weight by the conditional (matching the log), and eq 4's left-hand side reads
H(p(φ|c))instead of the impossibleH(p(c)). Both now agree with the implementation exactly.Verified before changing anything, on a small asymmetric 3×2 joint:
_clonotypic_one_phenotypic_one_mi_from_jointAdded tests that transcribe eqs 2–6 literally from the manuscript and assert the code computes exactly that. These become the primary definitional tests; the existing identity tests stay but they pin consequences of a formula (uniform → log₂k, degenerate → 0) rather than the formula itself.
SOURCE_ERRATAremoved from the manifest,__all__, bothnote_eqreferences, the module docstring,METRICS_CONTRACT.mdandCLAUDE.md. A short historical note remains in the prose so a reader holding the old revision isn't confused.test_note_literal_formula_would_break_the_decompositionis reframed rather than deleted, astest_marginal_weighting_is_not_an_entropy— marginal-weighting is the natural way to mis-transcribe these equations and it fails two ways at once: the value can exceedlog₂|C|(impossible for an entropy over|C|outcomes), and it makes the MI decomposition return a negative mutual information. Worth guarding independent of any manuscript revision.One live deviation, now explicit
Eq 6 defines
NMI = I / (½(H(c)+H(φ)))— the mean denominator — while tcri defaults to"min". On the contract's test joint that's 0.239 (eq 6) vs 0.293 (default), ~23% apart.minis deliberate: the mean denominator scales withlog₂(C)and so isn't comparable across groups with different clone counts.Recorded as
SANCTIONED_EXTENSIONS['normalize_mode_default']and pinned bytest_eq6_nmi_is_the_average_denominator, which asserts both that"average"reproduces eq 6 and that the default does not — so the divergence can never go silent. Anything reproducing the note's benchmark must passnormalize_mode="average".Metrics contract 12 → 17 tests. Full suite 169 passed, 3 skipped.