docs: state what the doc-anchor gate does not prove, and name both absences - #154
Merged
Conversation
…sences Two small things, neither changing behaviour. check-doc-anchors.py has to work out which symbol an anchor means, and for CODEMAP's tables that is a guess. Those rows label the link file.py:123 and put the symbol in a separate column, so _wanted_symbol takes the first plausible backticked name in the row. A row naming two symbols can be checked against the wrong one, which passes when that other symbol happens to sit on the anchored line. That was documented nowhere a reader would hit it. _wanted_symbol's docstring explained its fallback mechanism without saying the choice is a guess, and DEVELOPMENT.md named the hook among the prek gates and said nothing about its guarantee. The gate now sits under 94 anchors and caught real rot during the 7.2.0 doc work, so its reputation was running ahead of what it can prove. Both places say so now: a green run means no anchor is provably wrong, not that every anchor is provably right. The fix is to label each link with its own symbol, at which point the label is authoritative and the row is never consulted. That is roughly 120 links and is not done here; the limitation is recorded instead of quietly relied on. load_gemma_assoc read optional columns through _opt_float and then, on the next two lines, beta and se through an inline float(row.get(name, "nan")). Two idioms sat side by side for what looked like one idea. It is not one idea: an absent optional column is None because the test does not report it, and an absent beta is NaN because AssocResult requires it and GEMMA's LRT formats omit it. Both now go through a named helper, _opt_float and _float_or_nan, so the difference is stated rather than inferred from which idiom the author reached for. The helper cost 12 lines, which shifted three CODEMAP anchors into compare.py. The gate caught them; corrected here. Verified with the same pin as #152: compare_assoc_results over all seven committed .assoc.txt fixtures, four conditions each, digesting the rendered AssocComparisonResult. All 28 digests identical to master. pytest tests/ - 2275 passed, 7 skipped, 3 xfailed. prek run --all-files - all hooks pass, check-doc-anchors at 94 OK.
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
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.
Two small things, neither changing behaviour. Closing out the two remaining items from the thermo-nuclear review, one by fixing it and one by recording it rather than fixing it.
The gate's guarantee was overstated by omission
check-doc-anchors.pyhas to work out which symbol an anchor means. For CODEMAP's tables that is a guess: the rows label the linkfile.py:123and put the symbol in a separate column, so_wanted_symboltakes the first plausible backticked name in the row. A row naming two symbols can be checked against the wrong one, which passes when that other symbol happens to sit on the anchored line.That was documented nowhere a reader would hit it.
_wanted_symbol's docstring explained the fallback mechanism without ever saying the choice is a guess, and DEVELOPMENT.md listed the hook among the prek gates and said nothing about its guarantee.That mattered more after last week's work. The gate now sits under 94 anchors and caught real rot twice during the 7.2.0 doc pass, so its reputation had started running ahead of what it can prove. Both the module docstring and DEVELOPMENT.md now say it plainly: a green run means no anchor is provably wrong, not that every anchor is provably right.
The fix is not in this PR, deliberately. Labelling each link with its own symbol makes the label authoritative and the row irrelevant, but that is roughly 120 links across the docs. Not worth it today. Recording the limitation is, because the alternative is quietly relying on it.
Two absences, now both named
load_gemma_assocread optional columns through_opt_floatand then, on the next two lines, beta and se through an inlinefloat(row.get(name, "nan")). Two idioms side by side for what looked like one idea.It is not one idea. An absent optional column is
None, because the test does not report it. An absent beta is NaN, becauseAssocResultrequires it and GEMMA's LRT formats omit it. So this is not one helper with the duplicate deleted; it is_opt_floatand_float_or_nan, with the difference stated rather than inferred from which idiom the author happened to reach for.The gate caught me
_float_or_nancost 12 lines, which shifted three CODEMAP anchors intocompare.py.check-doc-anchorsfailed and named all three. Corrected here. That is the third time in a week it has caught a shift a human would not have, which is also the argument for being honest about where it cannot.Verification
Same pin as #152:
compare_assoc_resultsover all seven committed.assoc.txtfixtures, four conditions each, digesting the renderedAssocComparisonResultwith skip messages included. All 28 digests identical to master, so the helper swap changed nothing.pytest tests/: 2275 passed, 7 skipped, 3 xfailedprek run --all-files: all hooks pass,check-doc-anchorsat 94 OKpyrefly check: 0 errors, 43 suppressed, unchangedNot done, and I would leave it
The third item from that review was extracting the
.famparsing subtree out ofpipeline.py. I would drop it rather than carry it. The only reason it was on the list is that the file was over 1000 lines, and #151 took it to 897.parse_phenotypesis monkeypatched as a runner method in four places intest_pipeline_helpers.py, so the extraction costs real test churn to buy cohesion in a file that already reads fine.