Skip to content

data_integration_and_stats: per-attribute validation (ADR 0006) - #5

Merged
lwaldron merged 2 commits into
mainfrom
adr-0006-article
Nov 10, 2026
Merged

lwaldron merged 2 commits into
mainfrom
adr-0006-article

Conversation

@lwaldron

Copy link
Copy Markdown
Member

The article called bugphyzz:::.validationData(), which bugphyzz 1.7.15 removed, so it no longer renders. It also described imputation as a per-value MCC rule, which bugphyzzExports ADR 0006 replaced.

  • Reads validation_summary.tsv from the installed package directly, with no internal call, and adds validation_attributes.tsv (per-attribute kappa and R2 with their lower bounds) behind a new validation_attributes_tsv parameter.
  • The two count rows now read "attributes with imputed (ASR) annotations" and "attributes cross-validated for imputation", computed from the per-attribute table.
  • Supplementary Table 1's validation column gives the metric, mean, lower bound and the imputed or not-imputed decision, instead of a per-value score list.
  • slice_max(with_ties = FALSE) when ordering the validation figures: two values of one attribute can share the top score (COGEM ratings 1 and 2 in v1.0.7), which duplicated a factor level and stopped the render.

Figures 2a and 2b still show per-value MCC and R2 from validation_summary.tsv. Whether the manuscript's Figure 2 should instead show the per-attribute score that now decides imputation is a manuscript question, tracked in waldronlab/bugphyzz-manuscript#17.

Rendered against the 2026-11-10 export (the first under ADR 0006): 466,663 annotations, 93,838 taxa, 33 attributes, 18 sources, 59,521 imputed.

Drafted with Claude Code.

Reads validation_attributes.tsv instead of a removed bugphyzz internal,
counts attributes by the per-attribute rule, and reports the kappa lower
bound and decision in the supplementary table.

Co-authored-by: Claude Code <noreply@anthropic.com>
Comment on lines +81 to +87
numeric_attrs <- c("coding genes", "genome size", "growth temperature",
"length", "optimal ph", "width", "gc content")
validation_attr <- validation_attr |>
filter(rank == "all") |>
mutate(
threshold = ifelse(tolower(physiology) %in% numeric_attrs, 0.5, 0.7),
imputed = score_lcb >= threshold

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The numeric/discrete split is a hand-typed list of attribute names. An attribute that is missing from it, such as a new numeric attribute or a renamed one, gets the 0.7 threshold instead of 0.5. Its "imputed / not imputed" label in Supplementary Table 1 is then wrong, and nothing warns you. The table you just read already has a metric column (it is used in the sprintf below), so derive the threshold from that, e.g. threshold = ifelse(grepl("r2", metric, ignore.case = TRUE), 0.5, 0.7). Also, imputed only models the attribute-level rule. The export also requires row Score >= 0.5, which is not reflected here. The label is therefore "attribute passes", not necessarily "has imputed rows". Either say so or cross-check against attr_asr_default_names from bp.

)
}
## Per-attribute scores (ADR 0006): these decide which attributes are imputed
validation_attr <- if (nzchar(params$validation_attributes_tsv)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When bugphyzz_version pins a release, the per-attribute table still comes from the installed bugphyzz, unless validation_attributes_tsv is set. The existing validation_tsv branch warns about this mismatch, but this one doesn't. The "imputed / not imputed" column and the "cross-validated attributes" count can then disagree with the pinned bp data. Also, system.file() returns "" if the installed bugphyzz lacks the file (pre-1.7.15), and read_tsv("") fails with an unhelpful error. Add the same warning here, and stopifnot(nzchar(path)) with a message that asks for bugphyzz >= 1.7.15.

@claude

claude Bot commented Nov 10, 2026

Copy link
Copy Markdown

Two issues, both in vignettes/articles/data_integration_and_stats.Rmd (details are in the inline comments):

  1. The numeric/discrete threshold comes from a hard-coded attribute-name list. An attribute missing from it silently gets 0.7 instead of 0.5, which mislabels it as imputed or not imputed. Derive the threshold from the metric column instead. The label also ignores the row-level Score >= 0.5 rule.
  2. The per-attribute table always comes from the installed bugphyzz, even when bugphyzz_version pins a release. There is no mismatch warning, unlike the validation_tsv branch. system.file() returns "" if the file is missing, which gives an unhelpful error.

I found nothing else. This PR changes no exported functions or package code, so there are no Bioconductor documentation concerns.

…tion table

Co-authored-by: Claude Code <noreply@anthropic.com>
@lwaldron

Copy link
Copy Markdown
Member Author
  1. The threshold now follows the metric column, R2 at 0.5 and kappa or weighted kappa at 0.7, with a stopifnot on the metric values so a new one cannot pass silently. The prose says the attribute-level decision is subject to the row-level Score >= 0.5 rule.
  2. The per-attribute branch now warns when bugphyzz_version is not devel and the parameter is empty, matching the validation_tsv branch, and stops with a readable message if the installed bugphyzz predates 1.7.15.

Re-rendered against the 2026-11-10 export.

Drafted with Claude Code.

@lwaldron
lwaldron merged commit 5ce84ee into main Nov 10, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant