style: apply air formatter#173
Conversation
WalkthroughThe pull request predominantly reformats code and documentation across many R files. It also introduces targeted logic updates: enhanced subsetting ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant PS as PharmacoSet
participant Br as `[` method
participant ST as subsetTo
participant SST as .subsetToPharmacoSet
U->>Br: PS[i, j, ...]
alt i/j are character
Br->>ST: subsetTo(cells = i, drugs = j, ...)
else i/j are numeric integers
Br->>PS: sampleNames(x)[i], treatmentNames(x)[j]
Br->>ST: subsetTo(cells = names_i, drugs = names_j, molecular.data.cells = names_i)
end
ST->>SST: delegate with named args
SST-->>U: new PharmacoSet subset
sequenceDiagram
autonumber
actor C as Caller
participant S as summarizeSensitivityProfiles
participant LT as .summarizeSensProfiles (LongTable)
participant PS as .summarizeSensitivityProfilesPharmacoSet
C->>S: request summary (object, measure, ...)
alt object is LongTable
S->>LT: validate inputs, extract assay
LT->>LT: compute profiles, summarize, reshape
LT-->>S: matrix
else object is PharmacoSet
S->>PS: validate measure/cells/drugs
PS->>PS: build profiles, summarize, reshape
PS-->>S: matrix
end
S-->>C: summarized matrix
sequenceDiagram
autonumber
actor A as API Client
participant L as computeZIPdelta
participant C2 as .computeZIPdelta
participant Z as computeZIP
note over L,Z: New explicit per-treatment params
A->>L: call with HS/EC50/E_inf per treatment, doses, (optional ZIP)
alt ZIP provided
L->>C2: use provided ZIP reference
else no ZIP
L->>Z: compute ZIP reference surface
Z-->>L: ZIP
L->>C2: proceed with ZIP
end
C2->>C2: assemble combo profiles, compute deltas
C2-->>A: delta results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 41
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
R/computeGR.R (1)
40-56: Guard viability/Hill_fit before using viability; avoid error path.If both are missing,
logLogisticRegression(..., viability)will error before the explicit check. Add the check earlier and drop the later redundant check.Apply this diff:
- if (missing(Hill_fit)) { + if (missing(Hill_fit) && missing(viability)) { + stop("Please enter viability data and/or Hill equation parameters.") + } + if (missing(Hill_fit)) { Hill_fit <- logLogisticRegression( conc, viability, conc_as_log = conc_as_log, viability_as_pct = viability_as_pct, trunc = trunc, verbose = verbose, density = density, step = step, precision = precision, lower_bounds = lower_bounds, upper_bounds = upper_bounds, scale = scale, family = family )- if (missing(viability) && missing(Hill_fit)) { - stop("Please enter viability data and/or Hill equation parameters.") - }Also applies to: 80-82
R/methods-drugSensitivitySig.R (1)
321-329: Bug: misuse of do.call with colnames/rownamesdo.call(colnames, drugpheno.all) passes extra args to colnames; result is wrong and breaks drug/cell filters. Build unions explicitly.
- dix <- is.element(drugn, do.call(colnames, drugpheno.all)) + dix <- is.element( + drugn, + unique(unlist(lapply(drugpheno.all, colnames), use.names = FALSE)) + ) ... - cix <- is.element(celln, do.call(rownames, drugpheno.all)) + cix <- is.element( + celln, + unique(unlist(lapply(drugpheno.all, rownames), use.names = FALSE)) + )Also applies to: 330-338
🧹 Nitpick comments (79)
R/computeSynergy.R (7)
1124-1128: Fix error message: residual is character, not logical.The type check is correct, but the message is misleading.
- stop("argument `residual` must be type of logical") + stop("argument `residual` must be type of character")
271-275: Use scalar OR and handle NA, not just NaN, when checking tolerance.optimise() can return NA objective if the function evaluates to NA. Use || and include is.na().
- if (is.nan(guess_err) | guess_err > tol) { + if (is.na(guess_err) || is.nan(guess_err) || guess_err > tol) { loewe_estimate <- NA_real_ }
314-323: Doc/code mismatch: E_inf defaults.Docs state E_inf_1 and E_inf_2 default to 0, but the function has no defaults. Either set defaults or update docs. Setting defaults preserves back-compat.
-computeZIP <- function( +computeZIP <- function( treatment1dose, HS_1, EC50_1, - E_inf_1, + E_inf_1 = 0, treatment2dose, HS_2, EC50_2, - E_inf_2 + E_inf_2 = 0 ) {
392-394: Align hillCurve signature with docs: default E_ninf = 1.Docs say E_ninf defaults to 1; add the default for consistency.
-hillCurve <- function(dose, HS, EC50, E_inf, E_ninf) { +hillCurve <- function(dose, HS, EC50, E_inf, E_ninf = 1) { E_inf + ((E_ninf - E_inf) / (1 + (10^dose / 10^EC50)^(HS))) }
890-893: Avoid mutating global palette; use local colours.palette(rainbow(...)) changes the session-wide palette and returns the previous palette, not the new colours. Use grDevices::rainbow directly.
- cols <- palette(rainbow(length(unique_t2_dose))) + cols <- grDevices::rainbow(length(unique_t2_dose))- cols <- palette(rainbow(length(unique_t1_dose))) + cols <- grDevices::rainbow(length(unique_t1_dose))Also applies to: 975-977
414-421: .logcosh: numeric stability is good; minor cleanup optional.Looks fine. Optionally precompute ax <- abs(x) once to avoid recomputation.
1147-1153: Minor: message text grammar.“In during” reads odd.
- message(paste( - "ZIP reference values have not been pre-computed.", - "They will be computed in during delta score calculation." - )) + message(paste( + "ZIP reference values have not been pre-computed.", + "They will be computed during delta score calculation." + ))R/datasets.R (1)
6-8: Fix minor typos in roxygen docs.
- “most of of the dataset” → “most of the dataset”.
- “Sanger.This” → “Sanger. This”.
- Consider wrapping long reference lines for readability.
Apply this patch:
-#' group at the Broad Institute. This is not a full version of the dataset, most of -#' of the dataset was removed to make runnable example code. For the full dataset, +#' group at the Broad Institute. This is not a full version of the dataset; most of +#' the dataset was removed to make runnable example code. For the full dataset, @@ -#' to the Genomics of Drug Sensitivity in Cancer Project group at the Sanger.This is not a full version of the dataset, most of -#' of the dataset was removed to make runnable example code. For the full dataset, +#' to the Genomics of Drug Sensitivity in Cancer Project group at the Sanger. This is not a full version of the dataset; most of +#' the dataset was removed to make runnable example code. For the full dataset, @@ -#' group at the Broad Institute. This is not a full version of the dataset, most of -#' of the dataset was removed to make runnable example code. For the full dataset, +#' group at the Broad Institute. This is not a full version of the dataset; most of +#' the dataset was removed to make runnable example code. For the full dataset, @@ -#' \code{probeGeneMapping} +#' \code{probeGeneMapping}Also applies to: 24-27, 43-46, 60-63, 65-66, 74-74
R/globals.R (1)
2-16: Remove duplicate entries in globalVariables.'rn' appears twice; harmless but noisy.
utils::globalVariables(c( "X", "Y", "Cutoff", - 'rn', + 'rn', "treatmentid", "sampleid", - 'rn', 'rowKey', 'colKey', 'drug_cell_rep', 'value', 'max.conc', 'drug_cell_rep.x' ))R/plotPSig.R (2)
100-114: Use short‑circuit OR for scalar condition.Prefer || over | to avoid unintended vectorization.
- if (!missing(effect_cutoff) | !missing(signif_cutoff)) { + if (!missing(effect_cutoff) || !missing(signif_cutoff)) {
115-122: Optional: make aesthetics explicit to appease R CMD check.Using .data pronoun avoids NSE notes for X/Y/Cutoff.
- plot.elements <- plot.elements + - geom_point(aes(X, Y, color = Cutoff), data = x.m) + plot.elements <- plot.elements + + geom_point(aes(.data$X, .data$Y, color = .data$Cutoff), data = x.m)And similarly for the non‑cutoff branch:
- geom_point(aes(X, Y), color = color, data = x.m) + geom_point(aes(.data$X, .data$Y), color = color, data = x.m)R/geneDrugSensitivityPBCorr.R (2)
32-32: Remove unused variable.num.larger is never used; cur_success is returned instead.
-num.larger <- 0
241-253: Tighten error messages and fix typos.Minor text fixes improve clarity.
- "Molecular Feature is not discrete, but point biserial correlation was requested" + "Molecular feature is not discrete, but point biserial correlation was requested" @@ - 'More than two discrete settings for moleuclar feature not currently supported' + 'More than two discrete settings for molecular feature are not currently supported' @@ - "Currently only continous output allowed for point biserial correlations" + "Currently only continuous output allowed for point-biserial correlations"Also applies to: 258-262, 246-249
R/connectivityScore.R (1)
7-18: Wrapper looks good; consider explicit argument matching.Add match.arg for early validation and clearer errors.
connectivityScore <- function( x, y, method = c("gsea", "fgsea", "gwc"), nperm = 1e4, nthread = 1, gwc.method = c("spearman", "pearson"), ... ) { - CoreGx::connectivityScore(x, y, method, nperm, nthread, gwc.method, ...) + method <- match.arg(method) + gwc.method <- match.arg(gwc.method) + CoreGx::connectivityScore(x, y, method, nperm, nthread, gwc.method, ...) }R/matthewCor.R (1)
1-2: Fix typos in header/docs.“correlatipon” → “correlation”; “Mathews” → “Matthews”.
-## Matthews correlatipon coefficient -#' Compute a Mathews Correlation Coefficient +## Matthews correlation coefficient +#' Compute a Matthews Correlation CoefficientR/cosinePerm.R (2)
1-2: Doc typo.“Permuations” → “Permutations”.
-#' Cosine Permuations +#' Cosine Permutations
7-15: Optional: validate ‘alternative’.Do match.arg locally for clearer errors.
cosinePerm <- function( x, y, nperm = 1000, alternative = c("two.sided", "less", "greater"), include.perm = FALSE, nthread = 1 ) { - CoreGx::cosinePerm(x, y, nperm, alternative, include.perm, nthread) + alternative <- match.arg(alternative) + CoreGx::cosinePerm(x, y, nperm, alternative, include.perm, nthread) }tests/testthat/test_sanitizeInput.R (1)
3-5: Fix test description typo.“funcitons” → “functions”.
-context( - "Checking the sanitization of input to curve fitting and sensitivity summary funcitons" -) +context( + "Checking the sanitization of input to curve fitting and sensitivity summary functions" +)R/getRawSensitivityMatrix.R (2)
5-23: Avoid eval(parse()) for filtering; build logical index instead.Safer, faster, and easier to maintain.
- cond <- "sensitivityInfo(pSet)$sampleid == cell.id" - if (!missing(quality)) { - if (is.element("quality", colnames(sensitivityInfo(pSet)))) { - cond <- paste( - cond, - "sensitivityInfo(pSet)$quality == quality", - sep = " & " - ) - } - } - if (!missing(max.conc)) { - if (is.element("max.conc", colnames(sensitivityInfo(pSet)))) { - if (length(max.conc) > 1) { - max.conc <- paste(max.conc, collapse = "///") - } - cond <- paste( - cond, - "sensitivityInfo(pSet)$max.conc == max.conc", - sep = " & " - ) - } - } + sinfo <- sensitivityInfo(pSet) + cond_idx <- sinfo$sampleid == cell.id + if (!missing(quality) && "quality" %in% colnames(sinfo)) { + cond_idx <- cond_idx & sinfo$quality == quality + } + if (!missing(max.conc) && "max.conc" %in% colnames(sinfo)) { + mc <- if (length(max.conc) > 1) paste(max.conc, collapse = "///") else max.conc + cond_idx <- cond_idx & sinfo$max.conc == mc + } if (length(drug.id) > 1) { drug.id <- paste(drug.id, collapse = "///") } - cond <- paste( - cond, - "sensitivityInfo(pSet)$treatmentid == drug.id", - sep = " & " - ) - - exp.id <- which(eval(parse(text = cond))) + cond_idx <- cond_idx & sinfo$treatmentid == drug.id + exp.id <- which(cond_idx)Also applies to: 29-36
40-41: Prefer grepl with fixed = TRUE.Simpler and avoids regex overhead.
- if (length(grep("///", drug.id)) > 0) { + if (grepl("///", drug.id, fixed = TRUE)) {tests/testthat/test_summarizeMolecularProfiles.R (2)
10-11: Comment typo.“defition” → “definition”.
- ## FIXME:: No method defition for summarizeMolecularProfiles with class 'missing' + ## FIXME:: No method definition for summarizeMolecularProfiles with class 'missing'
62-70: Select the actual “last” replicate, not “all but first”.
[, -1]drops only the first column; use the last column explicitly.testSummary <- summarizeMolecularProfiles( GDSCsmall, "rna", summary.stat = "last" ) expect_equal( SummarizedExperiment::assay(testSummary, 1)[, "647-V"], - testCells[, -1] + testCells[, ncol(testCells)] )R/methods-summarizeMolecularProfiles.R (4)
21-21: Roxygen typo.“caracter” → “character”.
-#' @param features \code{caracter} A vector of the feature names to include in the summary +#' @param features \code{character} A vector of the feature names to include in the summary
70-72: Use boolean-safe check.Prefer
if (!isTRUE(summarize))to avoid non-boolean pitfalls.- if (summarize == FALSE) { + if (!isTRUE(summarize)) { return(molecularProfilesSlot(object)[[mDataType]]) }
116-121: Use newline ‘\n’ in warnings, not carriage return ‘\r’.Improves portability/log readability.
- warning( - "The passed in molecular data had no column: xptype. - \rEither the mDataType does not include perturbations, or the PSet is malformed. - \rAssuming the former and continuing." - ) + warning( + "The passed in molecular data had no column: xptype.\n", + "Either the mDataType does not include perturbations, or the PSet is malformed.\n", + "Assuming the former and continuing." + )
222-228: Avoid apply() on S4 DataFrame; coerce safely.Prevents unintended type coersion and preserves rownames.
- pp2 <- apply(pp2, 2, function(x) { - if (is.factor(x)) { - return(as.character(x)) - } else { - return(x) - } - }) + pp2 <- as.data.frame(pp2, stringsAsFactors = FALSE) + pp2[] <- lapply(pp2, function(x) if (is.factor(x)) as.character(x) else x) + pp2 <- S4Vectors::DataFrame(pp2, row.names = rownames(pp2))R/computeSlope.R (1)
42-44: Avoid rbind in a loop; preallocate/vectorize.
This grows a matrix repeatedly. Build once from the vector.Apply:
- for (dose in concentration) { - most.sensitive <- rbind(most.sensitive, cbind(dose, 0)) - } + most.sensitive <- cbind(dose = concentration, zero = 0)R/filterNoisyCurves.R (3)
24-30: Minor: parameter spelling.
mean.viablity is misspelled (viability). Consider adding a new alias arg and deprecating the old to avoid breaking callers.
31-39: mclapply portability.
parallel::mclapply does not parallelize on Windows; it may error when mc.cores > 1. Consider BiocParallel::bplapply or a platform-aware fallback.
79-89: Empty-combination case.
If length(xx) < 3, tt becomes empty and max(numeric(0)) is -Inf. If that’s undesired, guard explicitly.- tt <- t(combn(seq_along(xx), 2, simplify = TRUE)) + if (length(xx) < 3) return(0) + tt <- t(combn(seq_along(xx), 2, simplify = TRUE))R/drugPerturbationSig.R (2)
110-137: Consider parallel-safe lapply.
You set mc.cores but use lapply(). If parallelizing is desired and portability matters, use BiocParallel::bplapply with a param.- mcres <- lapply( + # e.g., BiocParallel + # param <- if (nthread > 1) BiocParallel::MulticoreParam(nthread) else BiocParallel::SerialParam() + # mcres <- BiocParallel::bplapply( + mcres <- lapply(
141-152: Minor: avoid repeated featureInfo/molecularProfiles calls.
Cache feature rows once; reduces overhead.R/drugDoseResponseCurve.R (2)
429-433: Color palette length may be insufficient.
If length(doses) > 7, indexing mycol[i] yields NA. Scale palette to series count.- mycol <- RColorBrewer::brewer.pal(n = 7, name = "Set1") + nseries <- max(1L, length(doses)) + base <- RColorBrewer::brewer.pal(min(9, max(3, min(9, nseries))), "Set1") + mycol <- if (nseries <= length(base)) base else grDevices::colorRampPalette(base)(nseries)
498-557: Handle zeros/negatives for log-x plotting and fitting.
Protect plot/fit when any dose <= 0 to avoid log-axis/fitting errors.- points(doses[[i]], responses[[i]], pch = 20, col = mycol[i], cex = cex) + ok <- is.finite(doses[[i]]) & doses[[i]] > 0 & is.finite(responses[[i]]) + if (!any(ok)) next + points(doses[[i]][ok], responses[[i]][ok], pch = 20, col = mycol[i], cex = cex) @@ - log_logistic_params <- logLogisticRegression( - conc = doses[[i]], - viability = responses[[i]] + log_logistic_params <- logLogisticRegression( + conc = doses[[i]][ok], + viability = responses[[i]][ok] ) - log10_x_vals <- .getSupportVec(log10(doses[[i]])) + log10_x_vals <- .getSupportVec(log10(doses[[i]][ok]))R/downloadSignatures.R (1)
63-67: Fix function name in error text.Reference should be
availablePSets, notavailablePSet.- 'Unknown Dataset. Please use the `Dataset Name` column in the - data.frame returned by the availablePSet function to select a + 'Unknown Dataset. Please use the `Dataset Name` column in the + data.frame returned by the `availablePSets()` function to select aR/rankGeneDrugSensitivity.R (1)
65-67: Error text mentionsdurationwhich is not an argument.Clean up message to avoid confusion.
- "length of drugpheno, type, duration, and batch should be equal to the number of rows of data!" + "length of drugpheno, type, and batch should be equal to the number of rows of data!"tests/testthat/test_computeABC.R (3)
65-71: Split nested expect_warning(expect_error(...)) into discrete checks.Chaining expect_error inside expect_warning is brittle and can mask which condition failed. Assert them separately with explicit regex.
Apply:
- expect_warning(expect_error(computeABC( + expect_error(computeABC( conc1 = c(1, 2, 3), conc2 = c(1, -2, 3), viability1 = c(.50, .60, .70), viability2 = c(.40, .90, .10), viability_as_pct = TRUE - ))) #should complain + )) # should error + expect_warning( + computeABC( + conc1 = c(1, 2, 3), + conc2 = c(1, -2, 3), + viability1 = c(.50, .60, .70), + viability2 = c(.40, .90, .10), + viability_as_pct = TRUE + ), + "as_pct" + ) # should warn about as_pct
72-79: Pin warning text for viability_as_pct to avoid false positives.Add a regex to ensure the expected warning is actually emitted, not an unrelated one.
- expect_warning(computeABC( + expect_warning(computeABC( conc1 = c(1, 2, 3), conc2 = c(1, 2, 3), viability1 = c(.50, .60, .70), viability2 = c(.40, .90, .10), viability_as_pct = TRUE - )) #should complain + ), "as_pct") #should complain
5-16: Stabilize error-message expectations — messages verifiedSubstrings exist in the code: R/sanityCheck.R:199 ("Please pass in only one of 'Hill_fit' and 'viability'...") and R/sanityCheck.R:34 ("Log concentration vector is not of same length as viability vector."), with related "real numbers" / "viability_as_pct" checks in sanityCheck.R and computeAmax.R. Use expect_error(..., class=...) or a tighter regex (e.g. "Please pass in only one.*Hill_fit.*viability") instead of fragile substring matches.
tests/testthat/test_subsetTo.R (1)
6-16: LGTM — formatting readably expresses intent.Nice readability improvement; behavior unchanged. Optionally, assert dimension equality to catch accidental row/column drops.
expect_identical(dim(CCLEsmaller), dim(CCLEsmall))tests/testthat/test_logLogisticRegression.R (1)
7-19: Silence incidental console output per TODO.Wrap calls expected to succeed in suppressMessages(...) to keep CI logs clean without hiding warnings you explicitly test for.
- expect_error(logLogisticRegression(c(1, 2, 3), c(50, 60))) #should complain + expect_error(logLogisticRegression(c(1, 2, 3), c(50, 60))) # should complain - expect_warning(logLogisticRegression( + expect_warning(logLogisticRegression( c(1, 2, 3), c(70, 60, 50), viability_as_pct = FALSE )) #should complainAnd for value-returning expectations elsewhere, e.g.:
- expect_equivalent( - logLogisticRegression( + expect_equivalent( + suppressMessages(logLogisticRegression( seq(-10, 10, 0.1), .Hill(seq(-10, 10, 0.1), c(1, 0, 0)), conc_as_log = TRUE, viability_as_pct = FALSE - ), + )), list(1, 0, 0) )R/methods-intersect.R (1)
7-7: Track the TODO or remove if out of scope for this PR.Since this PR is “style-only,” either open an issue for LongTable intersection or drop the TODO to avoid stale debt.
I can open a follow-up ticket with a brief design sketch if helpful.
tests/testthat/test_PharmacoSetClass.R (2)
101-105: Remove duplicate assertion.Lines 101–102 are identical expect_true checks; keep one.
- expect_true(all(unique(sensitivityInfo(GDSCsmall)$sampleid) %in% newNames)) - expect_true(all(unique(sensitivityInfo(GDSCsmall)$sampleid) %in% newNames)) + expect_true(all(unique(sensitivityInfo(GDSCsmall)$sampleid) %in% newNames))
7-9: Consider migrating expect_equal_to_reference() to snapshots.If you move to testthat 3e, snapshot tests (expect_snapshot/expect_snapshot_file) are preferred over reference RDS fixtures.
Also applies to: 12-14, 17-21, 25-29, 33-37, 41-45, 49-53, 58-63, 67-71, 75-77, 81-83, 87-89
tests/testthat/test_computeIC50.R (2)
31-51: Add explicit warning regex per TODO; fix typo.Pin the expected warning text (e.g., "as_pct") and correct the “wanring” typo in the comment.
- ##TO-DO:: Add wanring strings to expect_warning call + ## TO-DO: Add warning strings to expect_warning call @@ - expect_warning( + expect_warning( computeIC50( concentration = c(1, 2, 3), viability = c(.50, .60, .70), viability_as_pct = TRUE ), - "as_pct" + "as_pct" ) #should complain
5-21: Stabilize error-message expectations; verify sources.Tests assert substrings "Please pass in only one" and "is not of same length". Those strings exist but are defined in R/sanityCheck.R (R/sanityCheck.R:199 — "Please pass in only one of 'Hill_fit' and 'viability'..."; R/sanityCheck.R:34 — "Log concentration vector is not of same length as viability vector." — similar message at R/computeAmax.R:54). Because computeIC50 calls sanityCheck, current tests will match, but the wording is brittle — switch to a regex match or, preferably, assert on a classed error (expect_error(..., class=...)) to avoid fragile exact-string dependencies.
R/downloadPSet.R (2)
24-29: Switch API URLs to HTTPS.Avoid plaintext HTTP to reduce MITM risk and CRAN NOTE surprises on external links.
- avail.psets <- fromJSON("http://www.orcestra.ca/api/pset/canonical") + avail.psets <- fromJSON("https://www.orcestra.ca/api/pset/canonical") ... - avail.psets <- fromJSON("http://www.orcestra.ca/api/pset/available") + avail.psets <- fromJSON("https://www.orcestra.ca/api/pset/available")
94-96: Fix typo in error message.“PharamcoSets” → “PharmacoSets”.
- 'Unknown Dataset. Please use the availablePSets() function for the table of available PharamcoSets.' + 'Unknown Dataset. Please use availablePSets() for the table of available PharmacoSets.'tests/testthat/test_downloadPSet.R (2)
5-7: Guard networked tests.
availablePSets()hits a remote API; add skips to keep CI/CRAN stable when offline.test_that("AvailablePset Works as Expected", { + skip_on_cran() + skip_if_offline() canonical_df <- availablePSets(canonical = TRUE)Also applies to: 34-38
23-30: Minor: prefer<-for assignment in tests (consistency).Pure style; keep if your formatter enforces
=for assignment.R/sanityCheck.R (2)
48-50: Fix typos in warning text.“correspoding” → “corresponding”.
- "Missing viability with non-missing concentrations values encountered. Removing concentrations values correspoding to those viabilities" + "Missing viability with non-missing concentrations values encountered. Removing concentration values corresponding to those viabilities"
38-41: Fix typos in warning text.- "Missing concentrations with non-missing viability values encountered. Removing viability values correspoding to those concentrations" + "Missing concentrations with non-missing viability values encountered. Removing viability values corresponding to those concentrations"tests/testthat/test_PharmacoSet_utils.R (1)
28-28: Replace empty placeholder test with an explicit skip or add assertions.Empty tests pass trivially and hide gaps.
-test_that('subsetByFeature works...', {}) +test_that('subsetByFeature works...', { + skip("TODO: add coverage for subsetByFeature") +})R/mergePSets.R (1)
4-12: LGTM: formatting-only; logic preserved.Optional: prefer
format(Sys.time(), "%Y-%m-%d")overdate()for ISO-like metadata.Also applies to: 24-68, 70-116, 118-166, 167-178
R/logLogisticRegression.R (5)
122-124: Also validate bounds vector lengths.We check
densitylength but notlower_bounds/upper_bounds. Add a length check to prevent silent NA indexing.if (length(density) != 3) { stop("Density parameter needs to have length of 3, for HS, Einf, EC50") } + if (length(lower_bounds) != 3 || length(upper_bounds) != 3) { + stop("lower_bounds and upper_bounds must each be length 3, for HS, Einf, EC50") + }
117-121: Redundant type check for family.After
match.arg,familyis guaranteed character. Theis.character(family)guard and its message (“Cauchy flag…”) are misleading.
63-65: Doc/API mismatch: “cauchy” vs “Cauchy”.Docs mention "cauchy" (lowercase) while
match.argexpects "Cauchy". Consider accepting both cases or aligning docs.Do callers in this package (and downstream) pass "cauchy" or "Cauchy"? If mixed, we can normalize input with:
- family <- match.arg(family) + family <- match.arg(tolower(family), c("normal", "cauchy")) + family <- if (identical(family, "cauchy")) "Cauchy" else "normal"Please confirm CoreGx::.fitCurve expects "Cauchy"/"normal".
Also applies to: 85-85
157-166: Unused return from CoreGx::.sanitizeInput.
sanitizeInputis called for side-effects only;cleanDatacomes from.reformatData. IfsanitizeInputis solely validation, add a brief comment; otherwise, consider removing to avoid confusion.
218-223: Attribute name differs from docs.Docs say “Rsquared” but code sets attribute “Rsquare”. Align to avoid user confusion.
Would you prefer to update the roxygen to “Rsquare” (non-breaking) or rename the attribute (potentially breaking)?
R/computeICn.R (2)
42-50: Use scalar logical operator for missing-argument checks.Prefer
&&to avoid unintended vectorized behavior (style/readability).- if (missing(Hill_fit) & !missing(concentration) & !missing(viability)) { + if (missing(Hill_fit) && !missing(concentration) && !missing(viability)) {
77-82: Clarify n semantics (percent inhibition vs. viability).
n <- n / 100thenn <- 1 - nimpliesnis percent inhibition (IC10 → 90% viability). Ensure docs/examples state this explicitly to prevent misuse.R/computeAUC_old.R (1)
41-62: Use sanitized log_conc consistently (avoid log10(conc + 1) vs log10(conc) mismatch).Current code mixes transforms: numerator uses log10(conc + 1) while denominator uses log10(conc). Prefer the sanitized log_conc from sanitizeInput for both integrals to avoid -Inf at 0 and ensure consistency. Also check length after sanitization.
Apply this diff:
- if (length(conc) < 2) { + if (length(log_conc) < 2) { return(NA) } ... - trapezoid.integral <- caTools::trapz( - log10(as.numeric(conc) + 1), - as.numeric(viability) - ) + trapezoid.integral <- caTools::trapz( + log_conc, + as.numeric(viability) + ) - AUC <- round( - 1 - - (trapezoid.integral / - trapz(log10(as.numeric(conc)), rep(100, length(viability)))), + ref_level <- if (viability_as_pct) 100 else 1 + AUC <- round( + 1 - trapezoid.integral / caTools::trapz(log_conc, rep(ref_level, length(viability))), digits = 2 )R/computeABC.R (1)
52-57: Use short‑circuit ops and guard against NA in min/max.Replace vectorized | with || for scalars and add na.rm=TRUE to avoid NA propagation.
Apply this diff:
- if (missing(conc1) | missing(conc2)) { + if (missing(conc1) || missing(conc2)) { stop( "Both Concentration vectors the drugs were tested on must always be provided." ) } - if (missing(Hill_fit1) | missing(Hill_fit2)) { + if (missing(Hill_fit1) || missing(Hill_fit2)) { ... } ... - if (max(log_conc1) < min(log_conc2) | max(log_conc2) < min(log_conc1)) { + if (max(log_conc1, na.rm = TRUE) < min(log_conc2, na.rm = TRUE) || + max(log_conc2, na.rm = TRUE) < min(log_conc1, na.rm = TRUE)) { return(NA) } else { - extrema <- sort(c( - min(log_conc1), - max(log_conc1), - min(log_conc2), - max(log_conc2) - )) + extrema <- sort(c( + min(log_conc1, na.rm = TRUE), + max(log_conc1, na.rm = TRUE), + min(log_conc2, na.rm = TRUE), + max(log_conc2, na.rm = TRUE) + ))Also applies to: 119-136
R/computeAmax.R (1)
61-67: Scalar logicals: use && and add an early insufficiency check after zero-removal.Prevents accidental vector recycling and clearer behavior on small inputs.
Apply this diff:
ii <- which(concentration == 0) if (length(ii) > 0) { concentration <- concentration[-ii] viability <- viability[-ii] } + if (length(concentration) < 2) { + if (verbose) warning("Insufficient non-zero concentrations for curve fitting") + x <- NA_real_ + names(x) <- "Amax" + return(x) + } ... - if (min(viability) < 0 & verbose) { + if (min(viability) < 0 && verbose) { warning("Warning: Negative viability data.") } - if (max(viability) > 100 & verbose) { + if (max(viability) > 100 && verbose) { warning("Warning: Viability data exceeds negative control.") }R/computeAUC.R (2)
53-62: Add explicit viability requirement when fitting without Hill_fit.This yields a clearer error than letting the fit call fail.
Apply this diff:
- if (area.type == "Fitted" && missing(Hill_fit)) { + if (area.type == "Fitted" && missing(Hill_fit)) { + if (missing(viability)) { + stop("To fit a curve (area.type='Fitted'), supply raw viability or provide Hill_fit.") + } Hill_fit <- logLogisticRegression( concentration, viability, conc_as_log = conc_as_log, viability_as_pct = viability_as_pct, trunc = trunc, verbose = verbose )
111-124: Float equality checks.
pars[2] == 1andpars[1] == 0are brittle with floating point fits. Consider tolerances (e.g.,isTRUE(all.equal(..., tol))). Non-blocking.R/PharmacoSet-utils.R (2)
68-73: Name the mDataTypes argument in callNextMethod.Avoids positional ambiguity if generic changes.
Apply this diff:
- function(x, features, mDataTypes) { - callNextMethod(x = x, features = features, mDataTypes) + function(x, features, mDataTypes) { + callNextMethod(x = x, features = features, mDataTypes = mDataTypes) }
82-95: Align subset method signature with others.Use
signature(x = 'PharmacoSet')for consistency and clearer dispatch.Apply this diff:
setMethod( 'subset', - signature('PharmacoSet'), + signature(x = 'PharmacoSet'), function(x, samples, treatments, features, ..., mDataTypes) { callNextMethod( x = x, samples = samples, treatments = treatments, features = features, ..., mDataTypes = mDataTypes ) } )R/methods-summarizeSensitivityProfiles.R (1)
94-96: Use match.arg for summary.stat for consistency and validation.Manual defaulting accepts invalid values silently.
- if (missing(summary.stat) || length(summary.stat) > 1) { - summary.stat <- 'mean' - } + summary.stat <- match.arg(summary.stat)And keep summary.function as-is.
Also applies to: 151-178
R/intersectPSets.R (1)
197-270: Molecular profiles intersection logic: nested sapply/grep blocks are hard to follow and drop NULLs inconsistently.Risk of returning lists with NULL elements to .intersectList/.unionList. Clean up by filtering empty hits before intersect/union.
I can follow up with a small refactor that collects candidate SEs per type, filters length>0, then maps to sampleid vectors prior to CoreGx operations. Let me know if you want a patch.
R/computeDrugSensitivity.R (2)
241-270: Common concentration range assumes dose ordering and may return an empty seq when series are decreasing or unsorted.Guard by sorting each dose vector and short-circuit if min.dose > max.dose.
- for (i in seq_len(length(doses))) { - common.ranges[[i]] <- doses[[i]][ + for (i in seq_len(length(doses))) { + di <- sort(as.numeric(doses[[i]])) + if (!is.finite(min.dose) || !is.finite(max.dose) || min.dose > max.dose) { + common.ranges[[i]] <- numeric(0) + next + } + common.ranges[[i]] <- di[ seq( - which.min(abs(as.numeric(doses[[i]]) - min.dose)), + which.min(abs(di - min.dose)), max( which( - abs(as.numeric(doses[[i]]) - max.dose) == - min(abs(as.numeric(doses[[i]]) - max.dose), na.rm = TRUE) + abs(di - max.dose) == + min(abs(di - max.dose), na.rm = TRUE) ) ) ) ] }
13-16: Typo in error message (“expriments”).Polish user-facing strings.
- stop("expriments is empty!") + stop("experiments is empty!")R/geneDrugSensitivity.R (1)
176-219: Binomial path: tryCatch returns character on warning; downstream code tests is(rr0, "character").Silent coercion to string on warnings can mask convergence failures. Prefer explicit flagging and early return.
- rr0 <- tryCatch( + rr0 <- tryCatch( try(glm( formula(drugpheno.1 ~ . - x), data = dd, model = FALSE, x = FALSE, y = FALSE, family = "binomial" )), - warning = function(w) { - if (verbose) { - ww <- "Null model did not convrge" - print(ww) - ... - return(ww) - } - } + warning = function(w) structure(list(error="warn", what="null", msg=conditionMessage(w)), class="gx-warn") ) ... - rr1 <- tryCatch( + rr1 <- tryCatch( try(glm( formula(drugpheno.1 ~ .), data = dd, model = FALSE, x = FALSE, y = FALSE, family = "binomial" )), - warning = function(w) { - if (verbose) { - ww <- "Model did not converge" - ... - } - return(ww) - } + warning = function(w) structure(list(error="warn", what="full", msg=conditionMessage(w)), class="gx-warn") )Then short-circuit if inherits(rr0,"gx-warn") || inherits(rr1,"gx-warn").
R/rankGeneDrugPerturbation.R (1)
36-44: nthread guard can be simplified; missing(nthread) is never TRUE due to default.Small cleanup; no behavior change.
- if (nthread != 1) { - availcore <- parallel::detectCores() - if (missing(nthread) || nthread < 1 || nthread > availcore) { + if (nthread != 1) { + availcore <- parallel::detectCores() + if (nthread < 1 || nthread > availcore) { nthread <- availcore } else { # print(paste("all",nthread,"cores have been allocated")) } }R/methods-subsetTo.R (2)
127-129: Error messages reference wrong helper namesUse sampleNames/treatmentNames to match APIs.
- 'Some of the cell names passed to function did not match to names in the PharmacoSet. Please ensure you are using cell names as returned by the cellNames function' + 'Some of the cell names passed did not match the PharmacoSet. Please use sampleNames(object).' ... - 'Some of the drug names passed to function did not match to names in the PharmacoSet. Please ensure you are using drug names as returned by the drugNames function' + 'Some of the drug names passed did not match the PharmacoSet. Please use treatmentNames(object).'Also applies to: 145-146
70-70: Clarify drop behaviorHard-coding drop = FALSE affects all downstream subsetting. Either expose as a formal argument or remove the TODO and document the choice.
R/methods-drugSensitivitySig.R (2)
403-461: Parallelism portability: mclapply is not supported on WindowsConsider BiocParallel or parallel::parLapply fallback when .Platform$OS.type == "windows".
215-236: Message text consistencyStops for rnaseq/isoform say “for rna”; adjust to the actual modality to reduce confusion.
Also applies to: 239-242
R/PharmacoSet-class.R (1)
336-353: Avoid warning() on success pathswarning(ifelse(...)) emits warnings even when OK. Prefer conditional warnings and messages.
- warning(ifelse( - nrow(rowData(profile)) != nrow(assays(profile)[[1]]), - sprintf( - '%s: number of features in fData is different from - SummarizedExperiment slots', - nn - ), - sprintf('%s: rowData dimension is OK', nn) - )) + if (nrow(rowData(profile)) != nrow(assays(profile)[[1]])) { + warning(sprintf( + "%s: number of features in fData differs from assay rows", nn + )) + } else { + message(sprintf("%s: rowData dimension is OK", nn)) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (65)
R/GR.R(1 hunks)R/GWC.R(2 hunks)R/PharmacoSet-accessors.R(5 hunks)R/PharmacoSet-class.R(6 hunks)R/PharmacoSet-utils.R(1 hunks)R/adaptiveMatthewCor.R(1 hunks)R/callingWaterfall.R(1 hunks)R/class-SignatureClass.R(4 hunks)R/computeABC.R(1 hunks)R/computeAUC.R(2 hunks)R/computeAUC_old.R(1 hunks)R/computeAmax.R(2 hunks)R/computeDSS.R(1 hunks)R/computeDrugSensitivity.R(2 hunks)R/computeGR.R(4 hunks)R/computeIC50.R(1 hunks)R/computeICn.R(2 hunks)R/computeSlope.R(1 hunks)R/computeSynergy.R(19 hunks)R/connectivityScore.R(1 hunks)R/cosinePerm.R(1 hunks)R/datasets.R(4 hunks)R/downloadPSet.R(2 hunks)R/downloadSignatures.R(2 hunks)R/drugDoseResponseCurve.R(1 hunks)R/drugPerturbationSig.R(2 hunks)R/filterNoisyCurves.R(1 hunks)R/geneDrugPerturbation.R(1 hunks)R/geneDrugSensitivity.R(2 hunks)R/geneDrugSensitivityPBCorr.R(6 hunks)R/geneDrugSensitivityPCorr.R(4 hunks)R/getRawSensitivityMatrix.R(1 hunks)R/globals.R(1 hunks)R/intersectPSets.R(2 hunks)R/logLogisticRegression.R(4 hunks)R/matthewCor.R(1 hunks)R/mergePSets.R(1 hunks)R/methods-[.R(1 hunks)R/methods-drugSensitivitySig.R(6 hunks)R/methods-intersect.R(1 hunks)R/methods-subsetTo.R(1 hunks)R/methods-summarizeMolecularProfiles.R(4 hunks)R/methods-summarizeSensitivityProfiles.R(2 hunks)R/plotPSig.R(2 hunks)R/rankGeneDrugPerturbation.R(1 hunks)R/rankGeneDrugSensitivity.R(3 hunks)R/sanityCheck.R(1 hunks)R/updateObject-methods.R(1 hunks)R/zzz.R(2 hunks)codecover.R(1 hunks)tests/testthat/test_Hill.R(1 hunks)tests/testthat/test_PharmacoSetClass.R(1 hunks)tests/testthat/test_PharmacoSet_utils.R(1 hunks)tests/testthat/test_computeABC.R(1 hunks)tests/testthat/test_computeIC50.R(1 hunks)tests/testthat/test_downloadPSet.R(1 hunks)tests/testthat/test_drugPerturbationSig.R(1 hunks)tests/testthat/test_drugSensitivitySig.R(1 hunks)tests/testthat/test_logLogisticRegression.R(1 hunks)tests/testthat/test_sanitizeInput.R(1 hunks)tests/testthat/test_subsetTo.R(1 hunks)tests/testthat/test_summarizeMolecularProfiles.R(2 hunks)tests/testthat/test_summarizeSensitivityProfiles.R(1 hunks)tests/testthat/test_unionList.R(1 hunks)vignettes/DetectingDrugSynergyAndAntagonism.R(4 hunks)
🔇 Additional comments (39)
R/computeSynergy.R (3)
666-669: fitTwowayZIP: column validation and error path LGTM.Required columns set and validated; aggregate calls are consistent with new parameterization.
Also applies to: 672-776
1667-1669: Bliss reference computation LGTM.Correct for viability-based responses.
1691-1693: HSA reference computation LGTM.Using min(viability_1, viability_2) is correct under viability-as-response convention.
codecover.R (1)
4-4: LGTM — formatting-only change.No functional impact; keeping covr.fix_parallel_mcexit enabled is fine for CI runs.
tests/testthat/test_unionList.R (1)
6-15: LGTM — style-only reflow.Assertions unchanged; tests remain clear.
Also applies to: 18-23
R/GWC.R (1)
2-4: LGTM — formatting-only.Public API unchanged; pass-through to CoreGx::gwc remains intact.
Also applies to: 14-28
R/adaptiveMatthewCor.R (1)
9-11: LGTM — formatting only.R/matthewCor.R (1)
7-9: LGTM — formatting only.R/methods-summarizeMolecularProfiles.R (2)
306-308: Assay name assignment — confirm single-assay assumption.
assayNames(res) <- <string>is fine for 1 assay; verify multi-assay objects aren’t passed here.
74-86: Feature fallback is sensible.Defaulting features to rownames(featureInfo) reads well with fill.missing behavior.
R/computeSlope.R (1)
20-25: Signature reformatting looks good.
No API changes detected; parameters preserved.R/callingWaterfall.R (1)
7-23: Pure formatting; OK.
No semantic changes; direct passthrough to CoreGx::callingWaterfall maintained.R/GR.R (1)
6-7: LGTM — formatting-only change.Whitespace change around exponent; no functional effect.
tests/testthat/test_logLogisticRegression.R (1)
73-101: Fix only if numerical diffs arise post-formatter — otherwise good.If underlying fit messages/rounding changed, these tight tolerances (1e-3) may flap. If you see flakes, relax to 1e-2. Otherwise keep as-is.
tests/testthat/test_Hill.R (1)
5-10: LGTM — tests remain focused and precise.No behavior change; coverage on edge cases (±Inf) intact.
tests/testthat/test_computeIC50.R (1)
55-114: LGTM — value tests look correct and precise.Good coverage across typical/edge cases (NA_real_, Inf, n=0).
tests/testthat/test_PharmacoSetClass.R (1)
116-119: Brittle exact-warning text — make the test robust
Repo search found no exact matches for "Duplicated ids passed to updateSampleId" or "Duplicated ids passed to updateTreatmentId". tests/testthat/test_PharmacoSetClass.R (lines 116–119 and 154–157) currently asserts exact warning text — replace with a regexp match (e.g. regexp = "Duplicated ids passed to update") or assert the warning class instead.R/downloadPSet.R (1)
31-40: Confirm stability of versionInfo column expansion.
avail.psets$dataset$versionInfomay expand to multiple columns indata.frame(...). Please verify the resulting column count and types are stable vs. tests (they assertncols = 8).tests/testthat/test_drugPerturbationSig.R (1)
7-15: LGTM: formatting-only change.R/updateObject-methods.R (1)
15-18: Name-space-wide rename is broad.
names(curation(pSet)) <- gsub("drug","treatment", ...)will rewrite any key containing “drug”. Confirm downstream code expects this. Otherwise, constrain to exact key matches.tests/testthat/test_PharmacoSet_utils.R (1)
8-15: LGTM: formatting aligns with updated style.R/computeICn.R (1)
90-95: LGTM — formula branch is consistent with Hill parameterization.No functional issues spotted.
tests/testthat/test_summarizeSensitivityProfiles.R (6)
19-44: LGTM — reformat improves readability; logic unchanged.
45-62: LGTM — mean summary expectations preserved.
63-79: LGTM — “first” summary pathway verified.
81-97: LGTM — “last” summary pathway verified.
103-110: LGTM — parameter behavior checks read cleanly after reflow.
114-118: LGTM — max.conc handling test reads clearer.R/computeDSS.R (1)
2-13: LGTM — formatting-only changes; flow preserved.No behavioral diffs observed.
Also applies to: 19-27, 28-34, 38-46, 51-54, 55-59, 61-71, 72-73, 82-89, 91-93
tests/testthat/test_drugSensitivitySig.R (2)
8-17: LGTM — baseline signature test remains stable.
24-35: Ensure MANOVA reference present and test is deterministic
- tests/testthat/drug.sensitivity.MANOVAGDSCSmall.rds is present in the repo.
- If drugSensitivitySig (or its internals/parallelism) is non‑deterministic, add a seed immediately before the call in tests/testthat/test_drugSensitivitySig.R (around lines 24–35), e.g.:
set.seed(123)R/PharmacoSet-accessors.R (1)
124-139: Accessor wrappers look good.Consistent callNextMethod usage and signatures; no functional concerns.
R/geneDrugPerturbation.R (1)
65-73: Fix 'tsat' → 'tstat' typos
- R/geneDrugPerturbation.R — line 69: change
"tsat" = NAto"tstat" = NA.- R/geneDrugPerturbation.R — line 99: change
"tsat" = mm$coefficients["concentration", "t value"]to"tstat" = mm$coefficients["concentration", "t value"].- R/geneDrugSensitivity.R — line 346: update the commented example (
"tsat") to"tstat"for consistency.R/geneDrugSensitivity.R (1)
150-160: Model matrices omit x in rr0 but include it in rr1; OK. Ensure ff0 matches constructed drugpheno names.When ncol(drugpheno) > 1, ff0 is a cbind of drugpheno.i; keep in sync with the new colname logic above.
Also applies to: 220-272
R/rankGeneDrugPerturbation.R (1)
121-133: Drug concentration rescaling uses a scalar multiplier but may mix units; confirm expected units (µM vs M).Multiplying by 10^6 assumes base units; ensure consistency with downstream geneDrugPerturbation.
R/methods-summarizeSensitivityProfiles.R (1)
197-205: Fix matrix construction & rownames; confirm orientationas.matrix() doesn't accept rownames — extract the first column as rownames, convert the remaining columns to a numeric matrix, set rownames, and transpose if the docs require cell lines as rows and drugs as columns.
File: R/methods-summarizeSensitivityProfiles.R (lines 197-205)
- profSummary <- dcast( + profSummary <- data.table::dcast( profSummary, get(treatment_col) ~ get(sample_col), value.var = 'V1' ) - summaryMatrix <- as.matrix(profSummary, rownames = 'treatment_col') - return(summaryMatrix) + rn <- profSummary[[1]] + mat <- as.matrix(profSummary[, -1, with = FALSE]) + rownames(mat) <- rn + # Optional: transpose if you want cell lines as rows per docstring + # mat <- t(mat) + return(mat)R/methods-subsetTo.R (1)
38-57: Scope creep vs. “style-only” PR titleThis adds a new S4 method and changes behavior. Please update the PR title/description or split functional changes from formatting.
R/methods-drugSensitivitySig.R (1)
380-389: Batch vector construction may misalignYou set names(batch) only for molcellx, then reindex by rownames(drugpheno). Verify lengths/names align for all datasets; else derive batch <- factor(phenoInfo(...)[molcellx,'batchid'])[rownames(drugpheno[[1]])].
R/PharmacoSet-class.R (1)
448-450: dim() looks goodReturns a concise, named vector; matches expectations.
| cat('PharmacoSet Name: ', attr(object, 'PSetName'), "\n") | ||
| cat('Signature Type: ', attr(object, 'SigType'), "\n") | ||
| cat("Date Created: ", attr(object, 'DateCreated'), "\n") | ||
| cat("Number of Drugs: ", dim(object)[[2]], "\n") | ||
| cat("Number of Genes/Probes: ", dim(object)[[1]], "\n") | ||
| } |
There was a problem hiding this comment.
Use S4 slots, not attributes, in show().
attr() won’t read S4 slots; prints will be empty/NA.
- cat('PharmacoSet Name: ', attr(object, 'PSetName'), "\n")
- cat('Signature Type: ', attr(object, 'SigType'), "\n")
- cat("Date Created: ", attr(object, 'DateCreated'), "\n")
+ cat('PharmacoSet Name: ', object@PSetName, "\n")
+ cat('Signature Type: ', object@SigType, "\n")
+ cat("Date Created: ", object@DateCreated, "\n")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cat('PharmacoSet Name: ', attr(object, 'PSetName'), "\n") | |
| cat('Signature Type: ', attr(object, 'SigType'), "\n") | |
| cat("Date Created: ", attr(object, 'DateCreated'), "\n") | |
| cat("Number of Drugs: ", dim(object)[[2]], "\n") | |
| cat("Number of Genes/Probes: ", dim(object)[[1]], "\n") | |
| } | |
| cat('PharmacoSet Name: ', object@PSetName, "\n") | |
| cat('Signature Type: ', object@SigType, "\n") | |
| cat("Date Created: ", object@DateCreated, "\n") | |
| cat("Number of Drugs: ", dim(object)[[2]], "\n") | |
| cat("Number of Genes/Probes: ", dim(object)[[1]], "\n") | |
| } |
🤖 Prompt for AI Agents
In R/class-SignatureClass.R around lines 70-75, the show() method is using
attr(object, ...) which does not read S4 slots and yields empty/NA; replace
those attr() calls with S4 slot accessors (e.g., slot(object, "PSetName") or
object@PSetName if the class defines that slot) for PSetName, SigType,
DateCreated and use dim(object) for counts as before; ensure you access the
correct slot names and handle missing slots gracefully (e.g., check for is.null
or existsSlot) before printing.
| op <- options() | ||
| options(mc.cores = nthread) | ||
| on.exit(options(op)) |
There was a problem hiding this comment.
nthread is ignored by bplapply; set a BiocParallel parameter explicitly.
Setting options(mc.cores) won’t affect bplapply unless using MulticoreParam/SerialParam.
- op <- options()
- options(mc.cores = nthread)
- on.exit(options(op))
+ BPPARAM <- if (nthread > 1) BiocParallel::MulticoreParam(workers = nthread) else BiocParallel::SerialParam()
...
- auc_recomputed_star <- unlist(
- bplapply(
+ auc_recomputed_star <- unlist(
+ BiocParallel::bplapply(
rownames(sensitivityRaw(pSets[[study]])),
- FUN = function(experiment, exps, study, dataset, area.type) {
+ FUN = function(experiment, exps, study, dataset, area.type) {
...
- },
+ },
exps = exps,
study = study,
dataset = sensitivityRaw(pSets[[study]]),
- area.type = area.type
+ area.type = area.type,
+ BPPARAM = BPPARAM
)
)Also applies to: 49-77
🤖 Prompt for AI Agents
In R/computeDrugSensitivity.R around lines 45-47 (and similarly lines 49-77),
the code sets options(mc.cores = nthread) which does not control bplapply;
instead create and pass an explicit BiocParallel parameter: load BiocParallel if
needed, build BPPARAM <- if (nthread > 1) MulticoreParam(workers = nthread) else
SerialParam(), then call bplapply(..., BPPARAM = BPPARAM); remove the
options(mc.cores) on.exit boilerplate or keep op restore only for other
unrelated options.
| verbose = TRUE, | ||
| trunc = TRUE |
There was a problem hiding this comment.
Don't hardcode verbose/trunc; pass through user-supplied args.
Currently forces verbose=TRUE and trunc=TRUE, ignoring function inputs.
Apply:
- verbose = TRUE,
- trunc = TRUE
+ verbose = verbose,
+ trunc = trunc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| verbose = TRUE, | |
| trunc = TRUE | |
| verbose = verbose, | |
| trunc = trunc |
🤖 Prompt for AI Agents
In R/computeIC50.R around lines 23 to 24 the call is hardcoding verbose = TRUE
and trunc = TRUE which overrides user-supplied arguments; change the call to use
the function's parameters (e.g. verbose and trunc) instead of literal TRUE
values so the passed-in arguments control behavior, and ensure the function
signature includes those parameters (with appropriate defaults) so they are
forwarded.
| slope.prime <- .optimizeRegression( | ||
| x = most.sensitive[, 1], | ||
| y = most.sensitive[, 2] | ||
| ) |
There was a problem hiding this comment.
Division-by-zero risk when normalizing by slope.prime.
If .optimizeRegression on a zero vector returns 0, slope/abs(slope.prime) yields Inf/NaN.
Apply:
- slope.prime <- .optimizeRegression(
+ slope.prime <- .optimizeRegression(
x = most.sensitive[, 1],
y = most.sensitive[, 2]
)
slope <- .optimizeRegression(x = concentration, y = viability)
- slope <- round(slope / abs(slope.prime), digits = 2)
+ denom <- abs(slope.prime)
+ if (is.na(denom) || denom < .Machine$double.eps) {
+ return(NA_real_)
+ }
+ slope <- round(slope / denom, digits = 2)Also applies to: 51-51
🤖 Prompt for AI Agents
In R/computeSlope.R around lines 46-49 (and also line 51) the code divides by
slope.prime without guarding against zero, which can produce Inf/NaN if
.optimizeRegression returns 0; modify the logic to test if abs(slope.prime) <=
tol (use a small tolerance like .Machine$double.eps * max(1, abs(slope.prime)))
and if true handle the case explicitly (e.g., set the normalized slope to NA or
0, or skip normalization) otherwise perform slope/abs(slope.prime); apply the
same guard at line 51 to avoid division-by-zero.
| assertLogical(is_pct, len = 1) | ||
| if (is_pct) { | ||
| viability <- viability / 100 | ||
| E_inf <- E_inf / 100 | ||
| } | ||
| EC50 * ((1 - viability) / (viability - E_inf))^(1 / HS) |
There was a problem hiding this comment.
Add input validation and numeric guards to effectToDose to avoid Inf/NaN.
Division by zero occurs when viability <= E_inf; HS <= 0 or EC50 <= 0 are invalid. Guard denominator and return NA for out-of-domain inputs.
effectToDose <- function(viability, EC50, HS, E_inf, is_pct = FALSE) {
- assertLogical(is_pct, len = 1)
+ assertLogical(is_pct, len = 1)
+ # Basic validation
+ checkmate::assertNumeric(viability, any.missing = TRUE)
+ checkmate::assertNumeric(EC50, lower = 0, any.missing = TRUE)
+ checkmate::assertNumeric(HS, lower = .Machine$double.eps, any.missing = TRUE)
+ checkmate::assertNumeric(E_inf, any.missing = TRUE)
if (is_pct) {
viability <- viability / 100
E_inf <- E_inf / 100
}
- EC50 * ((1 - viability) / (viability - E_inf))^(1 / HS)
+ denom <- viability - E_inf
+ # out-of-domain where denom <= 0 or viability not in (E_inf, 1]
+ ood <- (denom <= 0) | (viability > 1) | (viability < 0)
+ res <- EC50 * ((1 - viability) / pmax(denom, .Machine$double.eps))^(1 / HS)
+ res[ood] <- NA_real_
+ res
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assertLogical(is_pct, len = 1) | |
| if (is_pct) { | |
| viability <- viability / 100 | |
| E_inf <- E_inf / 100 | |
| } | |
| EC50 * ((1 - viability) / (viability - E_inf))^(1 / HS) | |
| assertLogical(is_pct, len = 1) | |
| # Basic validation | |
| checkmate::assertNumeric(viability, any.missing = TRUE) | |
| checkmate::assertNumeric(EC50, lower = 0, any.missing = TRUE) | |
| checkmate::assertNumeric(HS, lower = .Machine$double.eps, any.missing = TRUE) | |
| checkmate::assertNumeric(E_inf, any.missing = TRUE) | |
| if (is_pct) { | |
| viability <- viability / 100 | |
| E_inf <- E_inf / 100 | |
| } | |
| denom <- viability - E_inf | |
| # out-of-domain where denom <= 0 or viability not in (E_inf, 1] | |
| ood <- (denom <= 0) | (viability > 1) | (viability < 0) | |
| res <- EC50 * ((1 - viability) / pmax(denom, .Machine$double.eps))^(1 / HS) | |
| res[ood] <- NA_real_ | |
| res |
🤖 Prompt for AI Agents
In R/computeSynergy.R around lines 41 to 46, the effectToDose calculation can
produce Inf/NaN when EC50 <= 0, HS <= 0, or viability <= E_inf (causing a
zero/negative denominator); add input validation and numeric guards: check EC50
and HS are numeric and > 0, ensure viability and E_inf are numeric and if is_pct
convert as before, then if viability <= E_inf or the computed denominator
(viability - E_inf) is <= 0 return NA (or NA_real_) instead of proceeding, and
otherwise compute the dose; this prevents divide-by-zero and invalid
exponentiation results.
| if (!is.character(data)) { | ||
| res <- t(apply( | ||
| data[, x, drop = FALSE], | ||
| 2, | ||
| geneDrugSensitivityPCorr, | ||
| type = type, | ||
| batch = batch, | ||
| drugpheno = drugpheno, | ||
| verbose = verbose, | ||
| test = inference.method, | ||
| req_alpha = req_alpha | ||
| )) | ||
| } else { | ||
| res <- t(apply(data[, x, drop = FALSE], 2, function(dataIn) { |
There was a problem hiding this comment.
Type check applies to whole matrix, not the current column.
Use is.character(data[, x]) to route to PB corr for categorical features.
- } else if (modeling.method == "pearson") {
- if (!is.character(data)) {
+ } else if (modeling.method == "pearson") {
+ if (!is.character(data[, x])) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!is.character(data)) { | |
| res <- t(apply( | |
| data[, x, drop = FALSE], | |
| 2, | |
| geneDrugSensitivityPCorr, | |
| type = type, | |
| batch = batch, | |
| drugpheno = drugpheno, | |
| verbose = verbose, | |
| test = inference.method, | |
| req_alpha = req_alpha | |
| )) | |
| } else { | |
| res <- t(apply(data[, x, drop = FALSE], 2, function(dataIn) { | |
| if (!is.character(data[, x])) { | |
| res <- t(apply( | |
| data[, x, drop = FALSE], | |
| 2, | |
| geneDrugSensitivityPCorr, | |
| type = type, | |
| batch = batch, | |
| drugpheno = drugpheno, | |
| verbose = verbose, | |
| test = inference.method, | |
| req_alpha = req_alpha | |
| )) | |
| } else { | |
| res <- t(apply(data[, x, drop = FALSE], 2, function(dataIn) { |
🤖 Prompt for AI Agents
In R/rankGeneDrugSensitivity.R around lines 155–168, the conditional currently
checks is.character(data) which inspects the whole matrix; change it to check
the current column instead (e.g., is.character(data[, x]) or is.character(data[,
x, drop = TRUE])) so the branch for PB correlation is chosen per-feature. Update
the if/else condition to use that per-column check and keep the rest of the
logic unchanged.
| if (conc_as_log == FALSE) { | ||
| ii <- which(conc == 0) | ||
| if (length(ii) > 0) { | ||
| conc <- conc[-ii] | ||
| } | ||
| log_conc <- log10(conc) | ||
| } else { | ||
| log_conc <- conc | ||
| } | ||
| if (is.unsorted(conc)) { | ||
| myx <- order(conc) | ||
| conc <- conc[myx] | ||
| } | ||
| return(list("Hill_fit" = Hill_fit, "log_conc" = log_conc)) |
There was a problem hiding this comment.
Sort concentrations before computing log10 (Hill_fit branch).
You compute log_conc and then sort conc, leaving log_conc unsorted. Sort first, then compute; or reorder log_conc accordingly.
- if (conc_as_log == FALSE) {
- ii <- which(conc == 0)
- if (length(ii) > 0) {
- conc <- conc[-ii]
- }
- log_conc <- log10(conc)
- } else {
- log_conc <- conc
- }
- if (is.unsorted(conc)) {
- myx <- order(conc)
- conc <- conc[myx]
- }
+ if (conc_as_log == FALSE) {
+ ii <- which(conc == 0)
+ if (length(ii) > 0) conc <- conc[-ii]
+ }
+ if (is.unsorted(conc)) {
+ myx <- order(conc)
+ conc <- conc[myx]
+ }
+ log_conc <- if (conc_as_log == FALSE) log10(conc) else conc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (conc_as_log == FALSE) { | |
| ii <- which(conc == 0) | |
| if (length(ii) > 0) { | |
| conc <- conc[-ii] | |
| } | |
| log_conc <- log10(conc) | |
| } else { | |
| log_conc <- conc | |
| } | |
| if (is.unsorted(conc)) { | |
| myx <- order(conc) | |
| conc <- conc[myx] | |
| } | |
| return(list("Hill_fit" = Hill_fit, "log_conc" = log_conc)) | |
| if (conc_as_log == FALSE) { | |
| ii <- which(conc == 0) | |
| if (length(ii) > 0) conc <- conc[-ii] | |
| } | |
| if (is.unsorted(conc)) { | |
| myx <- order(conc) | |
| conc <- conc[myx] | |
| } | |
| log_conc <- if (conc_as_log == FALSE) log10(conc) else conc | |
| return(list("Hill_fit" = Hill_fit, "log_conc" = log_conc)) |
🤖 Prompt for AI Agents
In R/sanityCheck.R around lines 181 to 194, the code computes log_conc from conc
before sorting conc, which leaves log_conc out of sync with the later-sorted
conc; fix by sorting conc first (or capture the order and apply it to log_conc)
before computing log10 so that log_conc aligns with conc (i.e., move the
is.unsorted/order logic above the conc -> log_conc branch or recompute/reorder
log_conc after sorting).
| colnames(curation(pSet)$treatment) <- gsub( | ||
| "treatmentid", | ||
| "treatmentid", | ||
| colnames(curation(pSet)$treatment) | ||
| ) | ||
| } |
There was a problem hiding this comment.
gsub is a no-op; intended rename likely from drugid → treatmentid.
Current pattern and replacement are identical, so column names never change. Fix to map historical “drug”/“compound” id columns to “treatmentid”.
- colnames(curation(pSet)$treatment) <- gsub(
- "treatmentid",
- "treatmentid",
- colnames(curation(pSet)$treatment)
- )
+ colnames(curation(pSet)$treatment) <- gsub(
+ "^(drug|compound)_?id$",
+ "treatmentid",
+ colnames(curation(pSet)$treatment),
+ ignore.case = TRUE,
+ perl = TRUE
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| colnames(curation(pSet)$treatment) <- gsub( | |
| "treatmentid", | |
| "treatmentid", | |
| colnames(curation(pSet)$treatment) | |
| ) | |
| } | |
| colnames(curation(pSet)$treatment) <- gsub( | |
| "^(drug|compound)_?id$", | |
| "treatmentid", | |
| colnames(curation(pSet)$treatment), | |
| ignore.case = TRUE, | |
| perl = TRUE | |
| ) | |
| } |
🤖 Prompt for AI Agents
In R/updateObject-methods.R around lines 19–24, the gsub call is a no-op because
the pattern and replacement are identical; change it to remap historical
"drug"/"compound" id columns to "treatmentid" (e.g., replace patterns like
"drugid" and "compoundid" with "treatmentid"), making the gsub pattern match
those legacy names and assigning the result back to
colnames(curation(pSet)$treatment). Ensure the replacement handles both variants
(for example using a regex like "^(drug|compound)id$") so legacy columns are
renamed to "treatmentid".
| if (interactive() && is.null(options('bhklab.startup_'))) { | ||
| oldOpts <- options() | ||
| options(warn = -1) | ||
| on.exit(options(oldOpts)) |
There was a problem hiding this comment.
Startup guard never fires and the sentinel is reverted.
- is.null(options('bhklab.startup_')) is always FALSE (options() returns a list).
- on.exit(options(oldOpts)) undoes bhklab.startup_ = FALSE, so the sentinel won’t persist.
Apply:
- if (interactive() && is.null(options('bhklab.startup_'))) {
- oldOpts <- options()
- options(warn = -1)
- on.exit(options(oldOpts))
+ if (interactive() && is.null(getOption("bhklab.startup_"))) {
+ oldWarn <- getOption("warn")
+ options(warn = -1)
+ on.exit(options(warn = oldWarn), add = TRUE)
@@
- # Prevent repeated messages when loading multiple lab packages
- options(bhklab.startup_ = FALSE)
+ # Prevent repeated messages when loading multiple lab packages (persist this)
+ options(bhklab.startup_ = FALSE)Also applies to: 12-13, 28-30
🤖 Prompt for AI Agents
In R/zzz.R around lines 7-10 (also applies to 12-13 and 28-30): the startup
sentinel check uses options('bhklab.startup_') which always returns a list so
the null check never fires, and the on.exit(options(oldOpts)) call restores the
original options and thus removes the sentinel; fix by using
getOption('bhklab.startup_', NULL) (or getOption with a default) for the
presence check, set the sentinel with options(bhklab.startup_ = FALSE) before
registering on.exit, and when restoring options remove the bhklab.startup_ entry
from oldOpts (e.g. oldOpts <- options(); oldOpts$bhklab.startup_ <- NULL;
on.exit(options(oldOpts))) so the sentinel persists after startup.
| # delete the additional column if not needed | ||
| message("No technical replicates in this dataset!") | ||
| mathews_griner[["tech_reps"]] <- NULL | ||
| } |
There was a problem hiding this comment.
Bug: wrong column name when removing tech rep.
Created column is tech_rep, but removal targets tech_reps, leaving stale column.
- # delete the additional column if not needed
- message("No technical replicates in this dataset!")
- mathews_griner[["tech_reps"]] <- NULL
+ # delete the additional column if not needed
+ message("No technical replicates in this dataset!")
+ mathews_griner[["tech_rep"]] <- NULL📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # delete the additional column if not needed | |
| message("No technical replicates in this dataset!") | |
| mathews_griner[["tech_reps"]] <- NULL | |
| } | |
| # delete the additional column if not needed | |
| message("No technical replicates in this dataset!") | |
| mathews_griner[["tech_rep"]] <- NULL | |
| } |
🤖 Prompt for AI Agents
In vignettes/DetectingDrugSynergyAndAntagonism.R around lines 51–54, the code
removes a non-existent "tech_reps" column while the created column is named
"tech_rep"; update the removal to mathews_griner[["tech_rep"]] <- NULL (or use
NULL assignment with the exact column name or use names(mathews_griner) check
before deletion) so the stale column is actually removed.
| PharmacoSet <- function( | ||
| name, | ||
| molecularProfiles = list(), | ||
| sample = data.frame(), | ||
| treatment = data.frame(), | ||
| sensitivityInfo = data.frame(), | ||
| sensitivityRaw = array(dim = c(0, 0, 0)), | ||
| sensitivityProfiles = matrix(), | ||
| sensitivityN = matrix(nrow = 0, ncol = 0), | ||
| perturbationN = array(NA, dim = c(0, 0, 0)), | ||
| curationTreatment = data.frame(), | ||
| curationSample = data.frame(), | ||
| curationTissue = data.frame(), | ||
| datasetType = c("sensitivity", "perturbation", "both"), | ||
| verify = TRUE, | ||
| ... | ||
| ) { |
There was a problem hiding this comment.
datasetType should be normalized
Use match.arg early; subsequent conditionals read the user’s vector default otherwise.
PharmacoSet <- function(
name,
molecularProfiles = list(),
sample = data.frame(),
treatment = data.frame(),
sensitivityInfo = data.frame(),
sensitivityRaw = array(dim = c(0, 0, 0)),
sensitivityProfiles = matrix(),
sensitivityN = matrix(nrow = 0, ncol = 0),
perturbationN = array(NA, dim = c(0, 0, 0)),
curationTreatment = data.frame(),
curationSample = data.frame(),
curationTissue = data.frame(),
- datasetType = c("sensitivity", "perturbation", "both"),
+ datasetType = c("sensitivity", "perturbation", "both"),
verify = TRUE,
...
) {
+ datasetType <- match.arg(datasetType)🤖 Prompt for AI Agents
In R/PharmacoSet-class.R around lines 98 to 114, datasetType is defined as a
character vector default but not normalized, so downstream conditionals may see
the full vector; call match.arg on datasetType immediately at the start of the
function (e.g., right after the argument list begins execution) to collapse it
to a single validated value and then use that normalized datasetType in
subsequent logic; ensure match.arg uses the same choices as the default and
leave verify and ... handling unchanged.
1 participant
Just formatting the package code using a standard formatter.
Summary by CodeRabbit
New Features
Refactor
Breaking Changes
Improvements
Tests/Docs