Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent infinite (or otherwise invalid) Ri/Ci range estimates during the post-processing of iterateOverCi() results by filtering out unusable grid points before per-protein analysis.
Changes:
- Add a pre-filter step in
automatedAnalysisIteratedCi()to remove grid points where theerrorvalue isNA/NaN. - Minor roxygen/whitespace cleanups in
R/06_optimization.R.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 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,] | ||
| } |
There was a problem hiding this comment.
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).
| # 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.") | |
| } |
There was a problem hiding this comment.
I would rather like to keep the results instead of stopping.
| # 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,] |
There was a problem hiding this comment.
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).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.