Skip to content

feat: add pgx.preprocess() as single source of truth for count preprocessing#493

Open
ESCRI11 wants to merge 7 commits into
edgyfrom
feat/pgx-preprocess
Open

feat: add pgx.preprocess() as single source of truth for count preprocessing#493
ESCRI11 wants to merge 7 commits into
edgyfrom
feat/pgx-preprocess

Conversation

@ESCRI11

@ESCRI11 ESCRI11 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Extracts the normalization pipeline that lived in the Omics Playground Shiny upload
module (imputedX → normalizedX → cleanX) into one exported playbase function, so a
script or the compute endpoint reproduces the app's X exactly instead of only a
plain log2(counts+prior).

  • pgx.preprocess(counts, samples, contrasts, annot, options): NA handling, NPX/NPQ
    back-transform, negatives→0, log2 with data-driven/method prior, group-wise
    missingness filter, imputation, normalization (CPM/TMM/quantile/methylation/
    multi-omics), outlier-sample removal.
  • pgx.createPGX() gains an opt-in preprocess= arg: when set and X is NULL, X
    is built from raw counts via pgx.preprocess before de-duplication. Existing
    bare callers are unaffected (X=NULL without preprocess → old log2 fallback).
  • Parity test (test-pgx-preprocess.R, 23 assertions) vs an independent copy of the
    app reactives. Verified end-to-end that createPGX(counts, preprocess=opt) yields
    identical X/counts to createPGX(counts=pp$counts, X=pp$X).

…cessing

Extract the normalization pipeline that lived in the Omics Playground Shiny
upload module (upload_module_normalization.R: imputedX -> normalizedX ->
cleanX) into a single exported playbase function, so a script or the compute
endpoint reproduces the app's X exactly instead of only a plain log2(counts+p).

pgx.preprocess() runs, in order: NaN/Inf handling, NPX/NPQ back-transform,
negatives->0, optional zero-as-NA, log2 with data-driven/method prior,
group-wise missingness filter, imputation, normalization (CPM/TMM/quantile/
methylation/multi-omics), and outlier-sample removal.

pgx.createPGX() gains an opt-in `preprocess=` argument: when set and X is NULL,
X is built from raw counts via pgx.preprocess() before de-duplication (so counts
and X are averaged/uniquified together, matching the app). Existing bare callers
are unaffected — X=NULL without preprocess still yields the log2 fallback.

Parity harness (test-pgx-preprocess.R) asserts pgx.preprocess() matches an
independent copy of the app reactives across all settings (23 assertions).
Verified end-to-end: createPGX(counts, preprocess=opt) yields identical X and
counts to createPGX(counts=pp$counts, X=pp$X) on the bundled fixture.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@phisanti

Copy link
Copy Markdown
Contributor

is.multiomics() has two conflicting definitions in playbase, both @exported:

  • R/pgx-functions.R:7: is.multiomics(pgx) (takes a pgx object)
  • R/pgx-mofa.R:1914: is.multiomics(probes) (takes a character vector of row names)

This is.multiomics(probes) is the one in use (R/pgx-preprocess.R:83) due to the collate order (that in playbase is alphabetical, we do not use [at]include). We should decide which one makes sense and delete the other to prevent confusion.

@phisanti

Copy link
Copy Markdown
Contributor

Minor comment: In my last code review with Ivo, he recommended avoiding passing list as an argument, such as options = list()) without giving defaults.

@phisanti

Copy link
Copy Markdown
Contributor

The deduplication logic is applied inconsistently between pgx.createPGX() (in R/pgx-compute.R) and the new pgx.preprocess() (in R/pgx-preprocess.R).

    kk <- intersect(rownames(X), rownames(counts))
    X <- X[kk, , drop = FALSE]
    counts <- counts[kk, , drop = FALSE]

This runs unconditionally on every call. It exists to handle cases where a normalization step drops rows (methylation's BMIQ against its probe-annotation manifest is the one legitimate case), but intersect() returns unique names, and matrix indexing-by-name keeps only the first match for a repeated name. So if counts has duplicate feature names going in, this step silently collapses each duplicate pair down to one arbitrary row — before pgx.createPGX()'s dedup block ever runs.

Net effect: pgx.createPGX()'s average.duplicated/make_unique logic never sees the duplicates it's designed to handle, because pgx.preprocess() has already destroyed one of each pair with no warning.

@phisanti

Copy link
Copy Markdown
Contributor

Add message logging for diagnostics in case of crash. For example:

  
  info("input: ", nrow(counts), " features x ", ncol(counts), " samples")
  ...
  info("NA/Inf handling + log2 transform done; ", sum(is.na(X)), " NAs remain")
  ...
  if (isTRUE(opt$filter_missing)) {
    info("filtering by missingness (threshold=", opt$filter_threshold, "): ", sum(!sel), " feature(s) excluded")

@phisanti

Copy link
Copy Markdown
Contributor

norm_method = "reference" without ref_gene crashes with an unhelpful low-level error:

if (identical(opt$norm_method, "reference") && !isTruthy(opt$ref_gene)) {
  stop("[pgx.preprocess] norm_method = 'reference' requires options$ref_gene to be set")
}

This also can be addressed at the level of the upload board blocking the compute if no reference is provided.

@phisanti phisanti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved with minor comments

@ESCRI11
ESCRI11 requested a review from phisanti July 10, 2026 08:38
@ESCRI11

ESCRI11 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

@phisanti addressed all your comments except the defaults one given the conversation we had via private messages

@phisanti phisanti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved now

The cleanX realign used %in% (counts order) while leaving X in its
post-normalization order. For methylation BMIQ (reorders + drops rows
against its probe manifest) this left rownames(counts) != rownames(X),
tripping the element-wise assert in pgx.createPGX(). Branch on
anyDuplicated(): %in% for duplicate names (RNA-seq, order-preserving),
name-index for unique names so counts follows any reorder+drop.

Tests: assert counts/X stay row/col-aligned in every parity case; cover
all 7 normalizeExpression methods (adds maxSum, reference) and pin the
reference-without-ref_gene guard.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

2 participants