Feedback/santi changes - #1
Open
phisanti wants to merge 32 commits into
Open
Conversation
tests/test-simple.R referenced an unrelated sibling package (WGCNAplus) and other undeclared dependencies, and was picked up by R CMD check as a standalone test since it lived directly under tests/ rather than tests/testthat/. It errored immediately, so the real testthat suite never even got a chance to run under check.
Both tests in test-model.R passed pheno="pheno" to create_model(), but the function's phenotype-mode parameter is meta.type, not pheno. Every test in the suite was erroring at setup as a result.
apply(x, 1, stats::sd) without na.rm=TRUE returns NA for any feature with missing values, and order()'s default na.last=TRUE pushes those to the bottom, so head(..., ntop) silently excluded them regardless of true variance. Common with real proteomics/metabolomics layers.
sp_edge_weight() calls igraph::shortest_paths() from SOURCE/SINK, which only exist when the model was built with add.sink=TRUE (not the default). Fail fast with a clear message instead of surfacing igraph's opaque 'Invalid vertex names' error.
layout_multipartite_3d()'s tsne/umap branches called Rtsne::Rtsne() and uwot::umap() without either package declared anywhere, so a clean install failed with 'there is no package called uwot' the moment plot_3d()'s default path hit that branch. Add them as Suggests with requireNamespace() guards and clear error messages, matching how other optional heavy deps are usually handled here.
clust=c("svd","tsne","umap") was never resolved with match.arg(),
so omitting the argument left clust as the full length-3 vector and
the first clust=='tsne' comparison crashed trying to coerce a
length-3 logical to a scalar. Also gives a proper error for invalid
values instead of silently falling through to svd.
expandPhenoMatrix() returns NULL when no phenotype column has resolvable/varying groups (e.g. a single-condition dataset), and create_model() didn't check before doing t(Y) three lines later, crashing with an unrelated t.default() error instead of explaining what was actually wrong.
- PKGNAME was a copy-paste leftover from the wgcnaplus Makefile template - on this case-insensitive filesystem it resolved to the sibling wgcnaplus directory, so make build/check silently targeted the wrong package instead of failing loudly
- do.plot=TRUE only warned on an empty graph then crashed several lines later with an unrelated 'names attribute' coercion error - do.plot=FALSE silently returned instead, giving no signal the result was empty - replace both with one clear stop() the moment the graph is empty
- A[ii, jj] without drop=FALSE silently turns into a plain vector whenever a merged layer has exactly one row or one column - same class of implicit-dimension-collapse bug already fixed elsewhere; add drop=FALSE so a future refactor of the accumulation logic below can't silently start producing wrong values
- both the inter-layer mask loop and the edge-reduction loop used seq_len(length(x) - 1), which becomes seq_len(-1) and errors when there are 0-1 layer types (e.g. data$X supplied as an empty list) - guard each loop with length(x) >= 2 so a degenerate/minimal model is still constructible instead of erroring on an internal detail
svd(ff[[k]])$u on a 1-row matrix has only 1 column, so the [,1:2] subset threw 'subscript out of bounds' -- e.g. a PHENO layer collapsed to one contrast column with add.revpheno=FALSE. Place single-feature layers at a fixed (0,0) instead of a meaningless 1-feature SVD.
- rowMeans() on a 0-column matrix returns NaN with no warning when a phenotype is entirely one-sided (all samples same group) - that NaN silently propagated into fold changes and edge weights - stop early with a clear message instead
Rtsne requires perplexity < (nrow-1)/3 strictly, but the perplexity was computed as nn/3, which is always greater than that bound -- so clust='tsne' errored on every call past the nn>5 threshold instead of only at genuine edge cases.
- non-paired multi-omics (each layer measured on a different sample subset) merges through mofa.merge_data2 without error, but can leave most of the result NA with no signal to the caller - warn when fewer than half the samples have data in every layer, naming the NA fraction and complete-sample fraction
plotly is already a declared Import and every call site is properly namespaced with plotly::, so these did nothing. R CMD check flagged them: use :: or requireNamespace() instead of library/require.
plot_visgraph()/plot_3d() both honor a layout attribute pre-attached to the graph object, but nothing in this package ever sets one -- create_model()/solve() don't. Not dead code: it's a low-cost way for a caller to inject a precomputed layout once and have later plot calls pick it up automatically. Document it as such.
Had only @export, no title/description/param/return, so roxygen emitted no Rd page and R CMD check flagged it as an undocumented exported object.
create_model, plot_3d, plot_multipartite, plot_visgraph, and plotlyLasagna each had roxygen blocks missing @PARAM entries for some of their real arguments, flagged by R CMD check as undocumented. Add the missing entries, matching each function's existing doc style.
man/*.Rd had drifted from R/ source for some time (missing params,
missing examples, and in plot_3d.Rd a stale \item{pos} entry for a
parameter that no longer exists). Sync now that source docs are
complete.
- %>% is used throughout plot.R, but only worked because igraph transitively re-exports magrittr's pipe -- an implementation detail that could break silently if igraph ever drops it - add magrittr to Imports and importFrom it directly
- the inline apply(x, 1, stats::sd, na.rm=TRUE) top-ntop filter and the following mofa.topSD(xx, ntop) call did identical work - collapse to the single mofa.topSD call; verified selected features are unchanged on a toy dataset with injected NAs
tests/multipartite-moxbrca.pdf was a generated side effect of the now-removed test-simple.R dev script, not meant to be tracked. Ignore tests/*.pdf going forward.
- Makefile has GNU-only ifdef extensions R CMD check flags as a portability warning, but it's a dev convenience wrapper, not part of the installed package -- exclude it, like wgcnaplus already does for its own equivalent Makefile - also exclude other local-only files (CLAUDE.md/AGENTS.md, .semquery, .beads, local_data) that have no business in a tarball
apply(etype, 2, match, gr$layers) relies on etype being a 2-column matrix, but apply() silently collapses to a plain vector when there are 0 or 1 edges (not just 0, as first suspected -- a degenerate model with a single edge hits this on ordinary input). Handle those cases directly instead of relying on apply()'s matrix shape.
- all three were maintainability-audit hotspots (plot_3d cyclomatic 29, MI 39.6, worst in the file); the file as a whole failed its maintainability floor - extract data-prep (layout resolution, node frame building, edge selection, subgraph filtering, styling) into small internal helpers; leave each exported function as a thin call-then-render wrapper - zero behavior change: exported signatures untouched, verified via a 25-case snapshot harness comparing resolved plotly/visNetwork output before and after, plus byte-identical error messages (including call attribution) on all three stop() paths
- second-highest complexity hotspot in the package (cyclomatic 22, MI 46.4 risky), after plot_3d which was already split - extract one helper per filtering concern (layer resolution, node selection, regex filtering, top-N, weight normalization, min-rho zeroing, sign filtering, type filtering, final delete+prune) - zero behavior change: verified across 466 parameter combinations via identical() comparison of graph structure/attributes
- core per-phenotype function was the package's third complexity hotspot (cyclomatic 14, MI 46.6 risky); its complexity is likely why the sp.weight/add.sink and degenerate-phenotype bugs fixed earlier this session went unguarded for so long - extract sample masking, correlation, fold-change, node/edge value assignment, shortest-path weighting, and edge capping into 7 helpers; both crash guards fixed earlier stay in their own helper - zero behavior change: verified across 731 parameter combinations, including both guarded error paths
- layout_multipartite_3d, multisolve, plot_3d, plot_multipartite, plot_visgraph, plotlyLasagna, prune_graph had zero test coverage despite being where every real crash bug found this session lived - add regression tests for the specific fixed bugs (clust match.arg default, single-feature svd, empty-filter plot_multipartite, plot_3d's three stop() paths) so they can't silently regress - shared toy-model fixtures in helper-toy.R keep the suite fast
- both do the same thing (repeat each element twice consecutively) but mapply's per-call dispatch overhead makes it ~68x slower than the equivalent vectorized rep() for this - byte-identical output, verified via identical() on real pipeline data as well as isolated benchmarks
- matrixStats is already a declared Import; rowMaxs is a single vectorized C call vs apply()'s per-row dispatch overhead, ~27x faster in isolated benchmarks - rowMaxs doesn't name its output like apply() does, and the next line greps names(maxrho) for SOURCE/SINK -- restore names explicitly so that lookup keeps working - verified identical() on maxrho and every graph/X component, for both add.sink=TRUE and add.sink=FALSE
- rii/rjj found the nc-th largest |correlation| per row/column via apply(abs(R1), 1/2, function(r) tail(sort(r), nc)[1]), duplicated 4x across the inter- and intra-layer edge-reduction loops - matrixStats::rowOrderStats/colOrderStats compute the k-th smallest directly; k = pmax(length - nc + 1, 1) gives the nc-th largest, matching tail()'s behavior even when nc exceeds the row/col length - this is the largest apply()-related contributor to create_model()'s wall time per profiling; verified identical() output including the nc-exceeds-length edge case, across full graph/X comparisons
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.
I reviewed the package against more datasets than
mox-brcaand found a few issues:Besides that, I also tackled a number of minor issues, but those are mostly cosmetic. After all changes, I verified the output is identical to before.