Skip to content

fix bug that caused inf estimates for Ri or Ci ranges #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
biwenzel merged 4 commits into from
Apr 27, 2026
Merged

fix bug that caused inf estimates for Ri or Ci ranges #68

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14112ab
fix bug that caused inf estimates for Ri or Ci ranges
KarinSchork Apr 23, 2026
8775454
Update R/06_optimization.R
KarinSchork Apr 23, 2026
0f9042f
Update R/06_optimization.R
KarinSchork Apr 23, 2026
a5738db
add test case
KarinSchork Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions R/06_optimization.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@
#' x is a vector containing first the RiLog values and then the Ci values
#' (length 2*m if no Ci are fixed).
#'
#' For the equality constraint (eqfun with corresponding bound eqB), the
#' For the equality constraint (eqfun with corresponding bound eqB), the
#' difference of the sum of the Ci values and 1 is calculated. The bound is set
#' to 0, i.e. forcing the sum of the Ci to be 1. In case of fixed Ci values, the
#' sum of the fixed and the free Ci values is considered.
Expand Down Expand Up @@ -460,8 +460,8 @@
#' G <- graphs$sample1_sample2[[2]]
#' # small example with a small grid size
#' iterateOverCi(G, gridSize = 100)
#'
#' @importFrom checkmate assertClass assertFlag assertIntegerish assertList
#'
#' @importFrom checkmate assertClass assertFlag assertIntegerish assertList
#' assertNumeric checkTRUE
#' @importFrom igraph is_bipartite V
#' @importFrom pbapply pbmapply pboptions
Expand Down Expand Up @@ -524,12 +524,12 @@ iterateOverCi <- function(G,
#' The data.frame resulting from the
#' [bppg::iterateOverCi()] function,
#' filtered for a specific protein.
#' @param error_tol \strong{numeric(1)} \cr
#' @param error_tol \strong{numeric(1)} \cr
#' tolerance for the error term.
#' @param ratioLog_tol \strong{numeric(1)} \cr
#' @param ratioLog_tol \strong{numeric(1)} \cr
#' tolerance for the log protein ratios.
#'
#' @returns Vector with minimal error, estimate for Ri (single value
#' @returns Vector with minimal error, estimate for Ri (single value
#' or min/max), estimate for Ci (single value or min/max).
#'
.analyseResultSingleProt <- function(protNr, resProt,
Expand All @@ -553,7 +553,7 @@ iterateOverCi <- function(G,
res_tmp <- c(mean(e_tmp), RLog_tmp_mean_rounded, NA, NA, NA,
min(C_tmp), max(C_tmp), NA)
# all(R[ind_min_tol] == 0) |???
res_tmp[8] <- ifelse(length(indMinError) == nrow(resProt), 1, 4)
res_tmp[8] <- ifelse(length(indMinError) == nrow(resProt), 1, 4)
} else {
# case 2 and 5: range solution for Rlog
res_tmp <- c(mean(e_tmp), NA, min(RLog_tmp), max(RLog_tmp), NA,
Expand Down Expand Up @@ -604,7 +604,7 @@ iterateOverCi <- function(G,
#' Tolerance for a constant log2 protein
#' ratios. The default is 1e-6.
#'
#' @return A SummarizedExperiment object with one row for each protein in the
#' @return A SummarizedExperiment object with one row for each protein in the
#' assay.
#' @export
#'
Expand Down Expand Up @@ -638,7 +638,18 @@ automatedAnalysisIteratedCi <- function(G,
job.id <- NA
}

f <- function(x, res, error_tol, ratioLog_tol,
# filter res for potential NaNs in the error column (could occur if a Ci
# is estimated as 0)
ind_error_NA <- which(is.na(res$error))
if (length(ind_error_NA) > 0) {
if (verbose) {
message(paste0(length(ind_error_NA),
" grid points with NA or NaN error term were removed."))
}
res <- res[-ind_error_NA,]

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

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

New behavior is introduced to drop grid points with non-usable error values before analysis; there are existing testthat snapshots for automatedAnalysisIteratedCi(), but nothing here asserts correct handling when res$error contains NA/NaN/Inf. Please add a test case that injects non-finite errors into a small res input and verifies the function returns finite outputs (and behaves sensibly if all grid points are filtered).

Copilot uses AI. Check for mistakes.
}
Comment thread
KarinSchork marked this conversation as resolved.
Outdated

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

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

This removal only drops NA/NaN errors, but Inf/-Inf error values can still occur (e.g., from log2(0) in the error equation) and will propagate to min()/range calculations, which matches the PR’s “Inf estimates” symptom. Consider filtering on !is.finite(res$error) instead of is.na(res$error), and add a guard for the case where all rows are removed (to avoid min(numeric(0)) == Inf downstream).

Suggested change
# filter res for potential NaNs in the error column (could occur if a Ci
# is estimated as 0)
ind_error_NA <- which(is.na(res$error))
if (length(ind_error_NA) > 0) {
if (verbose) {
message(paste0(length(ind_error_NA),
" grid points with NA or NaN error term were removed."))
}
res <- res[-ind_error_NA,]
}
# filter res for non-finite values in the error column (NA, NaN, Inf,
# -Inf; could occur if a Ci is estimated as 0)
ind_error_non_finite <- which(!is.finite(res$error))
if (length(ind_error_non_finite) > 0) {
if (verbose) {
message(paste0(length(ind_error_non_finite),
" grid points with non-finite error term were removed."))
}
res <- res[-ind_error_non_finite,]
}
if (nrow(res) == 0) {
stop("All grid points were removed because the error term was non-finite.")
}

Copilot uses AI. Check for mistakes.

@KarinSchork KarinSchork Apr 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I would rather like to keep the results instead of stopping.


f <- function(x, res, error_tol, ratioLog_tol,
use_results_from_other_proteins) {

cols <- c("protein", "error", paste0("RLog", x), paste0("C", x))
Expand All @@ -650,7 +661,7 @@ automatedAnalysisIteratedCi <- function(G,
resProt2 <- res[res$protein != x, cols]
colnames(resProt2) <- c("protein", "error", "RLog", "C")
# remove too extreme Ci
resProt2 <- resProt2[resProt2$C > 0.01 | resProt2$C < 0.99,]
resProt2 <- resProt2[resProt2$C > 0.01 | resProt2$C < 0.99,]
Comment thread
KarinSchork marked this conversation as resolved.
Outdated
resProt <- rbind(resProt, resProt2)
}
return(.analyseResultSingleProt(x, resProt, error_tol = error_tol,
Expand All @@ -664,15 +675,15 @@ automatedAnalysisIteratedCi <- function(G,
use_results_from_other_proteins = use_results_from_other_proteins)

# comparison=rep(comparison, n),
RES_info <- data.frame(accession = accessions,
RES_info <- data.frame(accession = accessions,
graphID = rep(graphID, n), proteinNr = seq_len(n))

RES <- cbind(RES_info, as.data.frame(t(RES)))
rownames(RES) <- RES$accession
RES <- RES[, -1] # remove accession, as it is now in rownames

RES_SE <- SummarizedExperiment::SummarizedExperiment(
assays = list(results = RES),
assays = list(results = RES),
rowData = data.frame(accession = accessions),
colData = data.frame(colnames = colnames(RES)))
return(RES_SE)
Expand Down
Loading