Add comprehensive statistical tests value set - #77
Conversation
Adds src/valuesets/schema/statistics/statistical_tests.yaml with ten enums covering statistical hypothesis testing: - StatisticalTestEnum: ~60 tests drawn from the descendants of 'statistical hypothesis test' (OBI:0000673) in STATO, grouped by t-tests/z-tests, ANOVA, rank-based non-parametric tests, categorical and contingency table tests, goodness of fit, variance/sphericity, odds ratio homogeneity, post-hoc procedures, outlier detection, model comparison, survival/genetics, and correlation - TTestTypeEnum, NormalityTestEnum, HomoscedasticityTestEnum, PostHocTestEnum: focused subsets for common slot ranges - MultipleTestingCorrectionEnum: Bonferroni, Holm-Bonferroni, Benjamini-Hochberg, Benjamini-Yekutieli, FWER/FDR, alpha spending/investing - CorrelationCoefficientEnum: Pearson, Spearman, Kendall - TestTailednessEnum, DistributionalAssumptionEnum, ComparisonObjectiveEnum (superiority/non-inferiority/equivalence) All 127 CURIEs were resolved and label-checked against OLS; every 'meaning:' target's ontology label matches the permissible value key, title, or an alias. Mappings prefer STATO, falling back to OBI and NCIT where STATO has no term. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rchj9PETEATt5FtzG9rMzd
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
The claude-review job on PR #77 failed at SDK startup (1 turn, 1.7s, zero cost) because the OAuth token was invalid. No content changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rchj9PETEATt5FtzG9rMzd
|
Claude finished @cmungall's task in 5m 10s —— View job Code Review: Statistical tests value set
Overall this is a well-organized, high-value addition. Structure matches the sibling The issues below are mostly about mapping verifiability and value-set design, not schema validity. 🔴 Main concern: ~85 STATO CURIEs are unvalidated by the repo's tooling
The diff corroborates this: it adds 9 OBI and 4 NCIT cache rows but zero STATO rows, and there is no This isn't specific to this PR — # src/valuesets/validators/oak_config.yaml
OBI: sqlite:obo:obi
STATO: sqlite:obo:stato # <-- addthen re-run I could not verify the CURIEs myself — this runner has no outbound network and 🟠 Correctness / usability1. 2. Aliases attached to the wrong t-test variant — 3. Correlation tests close-mapped to correlation coefficients — 4. 5. 🟡 Design: abstraction levels are mixed within enums
Options: split the grouping terms into a Related: ~22 permissible values duplicate a 🔵 Nits
⚪ Scope / process
Bottom line: structurally sound and CI-green; I'd want item 🔴 (STATO added to |
Main change: register STATO in src/valuesets/validators/oak_config.yaml so the ~83 STATO CURIEs in statistical_tests.yaml are strictly validated and cached rather than silently skipped. cache/stato/terms.csv is the resulting machine-checked evidence. This also retroactively covers the pre-existing STATO mappings in bio/expression_units.yaml, data_science/binary_classification.yaml and medical/neuroimaging.yaml; the latter two already validate clean, and the two label mismatches it surfaced in expression_units.yaml (FPKM/RPKM) are fixed here by adding STATO's verbatim labels as aliases. Net effect on expression_units.yaml is 14 -> 12 errors; the remainder are pre-existing UO/NCIT mismatches unrelated to this branch. Other review fixes in statistical_tests.yaml: - BENJAMINI_HOCHBERG alias FDR -> BH-FDR, which collided with the sibling FALSE_DISCOVERY_RATE permissible value - Pearson/Spearman correlation tests: close_mappings -> related_mappings, since STATO:0000280 / STATO:0000201 denote coefficients (data items) while the permissible values denote tests (planned processes) - HOLM_FDR: note that Holm's step-down procedure controls FWER, not FDR, and that the label is STATO's own - Two-sample t-tests: note that "unpaired"/"independent samples" are ambiguous and that R and SciPy default to the Welch variant - DistributionalAssumptionEnum: note the deliberate approximation of using test classes for what is really a property of a procedure - StatisticalTestEnum / MultipleTestingCorrectionEnum: state that grouping classes are intentional coarse-annotation fallbacks, and that meanings repeated in the narrower enums are deliberate - Corrective aliases for Scheffe test and Least significance different test - Comment explaining the k/c spelling split on homoskedasticity Reverted the unrelated cache/pato and cache/so churn picked up by running the validator over the whole schema directory; cache changes are now limited to ncit, obi and stato terms this branch actually uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rchj9PETEATt5FtzG9rMzd
|
Claude finished @cmungall's task in 2m 42s —— View job Re-review after
|
| CURIE | line | asserted as |
|---|---|---|
NCIT:C53240 |
99 | ONE_SAMPLE_T_TEST |
NCIT:C53232 |
107 | PAIRED_T_TEST |
NCIT:C53247 |
227 | MANN_WHITNEY_U_TEST |
NCIT:C53246 |
235 | WILCOXON_SIGNED_RANK_TEST |
NCIT:C53248 |
245 | KRUSKAL_WALLIS_TEST |
NCIT:C53235 |
267 | CHI_SQUARE_TEST |
NCIT:C45725 |
300 | FISHERS_EXACT_TEST |
NCIT:C53250 |
376 | SHAPIRO_WILK_TEST |
NCIT:C53229 |
626, 965 | NON_PARAMETRIC_TEST |
The 4 NCIT CURIEs added to the cache this PR (C53244, C53249, C61594, C53230) are exactly the ones used as meaning:, which confirms the split. This is a repo-wide tooling gap rather than something this PR introduced — but since ~10% of this file's mappings live in close_mappings, it's worth either spot-checking those 9 by hand before merge or filing an issue to extend the evaluator. File the tooling gap →
⚪ Process notes (no action needed for this PR)
just validateis not in CI.main.yaml:55runsjust test, which is_test-schema(gen-project) + pytest + examples (justfile:88,192). The ontology validation recipe lives inproject.justfile:81and is never invoked by a workflow, socache/stato/terms.csvis a point-in-time artifact — nothing will catch drift if a STATO label changes. Not a blocker, just worth knowing that the cache's value depends on someone re-running the validator manually.- CI is still in progress for
a07d416(Build and testrun 30714232947). The previous commit passed, and the new commit only adds comments, notes, aliases and oneclose_mappings→related_mappingsrename, so I'd be surprised by a failure — but it isn't green yet as I write this.
Bottom line: the blocking concern from my last pass is properly resolved with verifiable evidence, and the follow-ups are all in. From my side this is ready to merge once Build and test goes green; the unvalidated close_mappings are a pre-existing tooling gap you may prefer to handle separately.
· Branch
|
Addressed in a07d416. Thanks — the 🔴 item was the right call and is now fixed properly. Two of the nits don't hold up, details below. 🔴 STATO validation — doneAdded Worth recording what this turned up repo-wide, since it wasn't what either of us predicted. I had held off on this change because I expected it to surface ~4 new label mismatches on the pre-existing STATO mappings. It surfaced exactly 2, both in Both CURIEs were correct; only the labels were unmatched. Fixed by adding STATO's verbatim labels as aliases. Note this means those 2 errors were already being reported before my config change — the dynamic 🟠 Fixed
2. t-test aliases — addressed, but not by adding the aliases to both. Doing that would recreate exactly the collision from item 1, one enum over. Instead both variants carry a note: the equal-variance one flags that "unpaired"/"independent samples" are ambiguous, and the Welch one records that it's R's and SciPy's default, so an unqualified "unpaired t-test" often means Welch. 🟡 Abstraction levelsKept the grouping terms and documented the intent in both enum descriptions — they're deliberate fallbacks for sources that report only "a test of normality was applied", with instructions to prefer the most specific value. Also noted that meanings repeated in the narrower range-constraining enums are intentional. 🔵 Nits — two are incorrectNon-ASCII apostrophe — not changing this, and the premise is wrong. The claim that these are "the only two non-ASCII characters in the entire Ångström, μm, °C, β, Ψ, and en-dashes inside aliases used for label matching ( More to the point,
Spelling drift — added a comment explaining the ⚪ ScopeReverted
Local: Generated by Claude Code |
|
CI is green on 🟠
|
| CURIE | predicate | owning PV | resolved label |
|---|---|---|---|
NCIT:C53240 |
close | ONE_SAMPLE_T_TEST |
One Sample t-Test |
NCIT:C53232 |
close | PAIRED_T_TEST |
Paired t-Test |
NCIT:C53247 |
close | MANN_WHITNEY_U_TEST |
Mann-Whitney Test |
NCIT:C53246 |
close | WILCOXON_SIGNED_RANK_TEST |
Wilcoxon Signed Rank Test |
NCIT:C53248 |
close | KRUSKAL_WALLIS_TEST |
Kruskal-Wallis Test |
NCIT:C53235 |
close | CHI_SQUARE_TEST |
Chi-Square Test |
NCIT:C45725 |
close | FISHERS_EXACT_TEST |
Fisher's Exact Test |
NCIT:C53250 |
close | KOLMOGOROV_SMIRNOV_TEST |
Komogorov-Smirnov Test |
NCIT:C53229 |
close | NON_PARAMETRIC_TEST |
Non-Parametric Test |
NCIT:C53229 |
close | NON_PARAMETRIC |
Non-Parametric Test |
STATO:0000280 |
related | PEARSON_CORRELATION_TEST |
Pearson's correlation coefficient |
STATO:0000201 |
related | SPEARMAN_CORRELATION_TEST |
Spearman's rank correlation coefficient |
All 12 resolve and all denote the intended concept. No changes needed.
One correction to the table in the re-review: NCIT:C53250 is attributed to SHAPIRO_WILK_TEST, but it sits on KOLMOGOROV_SMIRNOV_TEST (L369; L376 is the CURIE line inside that value). That distinction matters here — NCIT has no Shapiro-Wilk term at all, so had it actually been on SHAPIRO_WILK_TEST it would have been a genuine wrong-term mapping rather than a correct one. SHAPIRO_WILK_TEST carries only meaning: STATO:0000077, no close_mapping. Worth flagging since the whole point of the check was catching exactly that failure mode.
(NCIT's own label misspells it as "Komogorov" — that's upstream, and it doesn't affect us since close_mappings aren't label-matched.)
Two other small numbers in the re-review: cache/stato/terms.csv holds 83 terms (88 lines — 1 header, and 4 labels contain embedded newlines from STATO definitions), matching the 83 distinct STATO CURIEs used as meaning: in the file, not 87/84.
⚪ On just validate not being in CI
Confirmed and worth acting on separately — main.yaml runs just test, and the ontology validation recipe in project.justfile:81 is never invoked by any workflow. So the cache is point-in-time and label drift wouldn't be caught. Wiring just validate into CI isn't free right now: main currently has 138 pre-existing errors across other modules (UO unit labels, NCIT mismatches), so the job would need to start from a baseline-allowlist rather than a clean zero. That plus extending the evaluator to cover close_mappings/related_mappings is a reasonable follow-up issue — out of scope for this PR, and @cmungall's call whether to file it.
Generated by Claude Code
|
Correcting my own previous comment: the I said the file holds 83 terms and attributed the 88 lines to embedded newlines in STATO definitions. That was wrong on both counts — I inferred it from 87 is correct — the re-review's number, not mine. The four entries beyond this file's 83 are the pre-existing STATO mappings elsewhere, picked up when I ran the validator against those files to check the
So the cache is 83 + 4, and every STATO CURIE in the repo is now cached — which is the better outcome anyway, since it's what makes the retroactive coverage real rather than notional. The one number that is off the other way: distinct STATO CURIEs used as Nothing in the diff changes as a result — this is a correction to my description of the artifact, not to the artifact. Generated by Claude Code |
Summary
Adds a comprehensive value set for statistical hypothesis tests and related statistical concepts, and registers STATO with the repository's ontology validator so those mappings are actually checked.
Key Changes
New schema file:
src/valuesets/schema/statistics/statistical_tests.yaml— 10 enums:StatisticalTestEnumstatistical hypothesis test(OBI:0000673) in STATO — t/z tests, ANOVA, rank-based non-parametrics, categorical and contingency tests, goodness of fit, variance/sphericity, post-hoc, outlier detection, model comparison, survival/genetics, correlationTTestTypeEnumNormalityTestEnumHomoscedasticityTestEnumPostHocTestEnumMultipleTestingCorrectionEnumCorrelationCoefficientEnumTestTailednessEnumDistributionalAssumptionEnumComparisonObjectiveEnumValidator change: registered
STATO: sqlite:obo:statoinsrc/valuesets/validators/oak_config.yaml. STATO was previously unlisted, so its CURIEs were never strictly validated or cached — despite supplying most of the mappings in this file and appearing in three existing modules.cache/stato/terms.csv(87 terms) is the resulting machine-checked evidence.Retroactive fix: enabling STATO surfaced two label mismatches on pre-existing mappings in
bio/expression_units.yaml(FPKM, RPKM). Both CURIEs were correct; STATO's verbatim labels are added as aliases. That file goes from 14 to 12 errors — the remaining 12 are pre-existing UO/NCIT mismatches unrelated to this branch.data_science/binary_classification.yamlandmedical/neuroimaging.yamlalready validated clean.Schema integration: added the
statistics/statistical_testsimport tovaluesets.yaml.Cache updates:
ncit(+4),obi(+9), and the newstato(87) — limited to terms this branch uses.Verification
meaning:target's ontology label matches its permissible value key, title, or an alias.meaning:mappings (close_mappings/related_mappings) hand-verified against their owning permissible values — the evaluator only readspv.meaning, so nothing in the repo checks these automatically.just test: 63 passed, 1 skipped.just gen-projectclean.statistical_tests.yamlvalidates clean under strict STATO mode.Notes
Barlett's test,Yate's corrected Chi-Squared test,Least significance different test) because strict validation matches on the ontology label; corrected spellings are provided as aliases.regen-derived.yamlregenerates them on merge tomain.Known follow-ups (out of scope)
just validateis not wired into CI, so the term caches are point-in-time and label drift won't be caught.maincurrently has 138 pre-existing errors across other modules, so this would need a baseline allowlist first.enum_evaluator.pyvalidates onlypv.meaning, notclose_mappings/related_mappings/exact_mappings.https://claude.ai/code/session_01Rchj9PETEATt5FtzG9rMzd