fix: remove.na.rows() ignored the cols subset and crashed on single column - #64
Merged
Conversation
…olumn
Two bugs in one function:
1. The completeness check compared against NCOL(mat) (the full
matrix's column count), not the size of the requested cols subset:
idxOK <- which(rowSums(!apply(mat2, 2, is.na)) == NCOL(mat))
Whenever cols was a proper subset of all columns -- exactly what
the cols parameter exists for, per its own doc "Cols to check for
NAs" -- a row could have zero NAs in the checked columns and still
never satisfy rowSums(...) == NCOL(mat), because that per-row count
can never reach the *full* matrix's column count unless cols
happens to be all of them. Net effect: passing any cols narrower
than the whole matrix silently dropped every row that had an NA
anywhere outside the checked columns too -- rows the caller
explicitly asked not to check.
2. mat[, cols] used the default drop = TRUE, so a single-column cols
(e.g. cols = 2) collapsed to a plain vector, and
apply(mat2, 2, is.na) on a vector errors ("dim(X) must have a
positive length").
Fixed both: added drop = FALSE to the subsetting, and compare against
NCOL(mat2) (the actual subset size) instead of NCOL(mat).
Verified on a 4x3 matrix with one NA in column "a" (row r3) and one
NA in column "b" (row r2):
- Default (all columns): drops r2 and r3, keeps r1/r4 -- unchanged.
- cols = c(1,3) (columns a,c, neither has NA in the surviving rows'
intersection... actually columns a and c only, ignoring column b):
now correctly keeps r2 (its only NA is in column b, not checked) --
previously this configuration dropped every row, including ones
with zero NAs anywhere.
- cols = 1 (single column, a): previously crashed; now correctly
drops only r3 (the row with an NA in column a).
- cols = 2 (single column, b): previously crashed; now correctly
drops only r2 (the row with an NA in column b).
Also bumps Development/config.R/DESCRIPTION to 2.8.14 per repo
convention.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b676f1401
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
rowSums(!apply(mat2, 2, is.na)) crashed when mat2 had exactly one row, since apply() simplifies its result to a plain logical vector (no dimensions) in that case, so rowSums() has nothing to sum over. This included the single-column subset case this branch already fixes. Replace with rowSums(!is.na(mat2)), which never depends on apply()'s simplification behavior and gives the same result for all other cases. Verified: default all-cols, 2-col subset, single-col subset, one-row all-cols, one-row with NA, and one-row single-col subset (the exact crash Codex flagged) all now return correct results with no error.
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
2 participants
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.
What was the bug?
Two bugs in one function:
Wrong completeness check. The check compared the per-row non-NA count in the selected
colssubset againstNCOL(mat)— the full matrix's column count:Whenever
colswas a proper subset of all columns (exactly what thecolsparameter is documented for: "Cols to check for NAs"), a row's per-checked-column non-NA count can never reach the full matrix's column count unlesscolshappens to be every column. Net effect: passing anycolsnarrower than the whole matrix silently dropped rows based on NAs outside the columns the caller asked to check — or dropped every row entirely.Crash on a single-column subset.
mat[, cols]used the defaultdrop = TRUE, socols = 2(a single column) collapsed to a plain vector, andapply(mat2, 2, is.na)on a vector throwsdim(X) must have a positive length.The fix
drop = FALSEto the subsetting.NCOL(mat2)(the actual subset size) instead ofNCOL(mat).Testing
4x3 matrix, one NA in column
a(rowr3), one NA in columnb(rowr2):Default: drops
r2,r3, keepsr1/r4— unchanged.cols = c(1,3): now correctly keepsr2(its only NA is in columnb, not checked) — previously this dropped every row, including ones with zero NAs anywhere in the checked columns.cols = 1: previously crashed; now correctly drops onlyr3.cols = 2: previously crashed; now correctly drops onlyr2.R CMD build+R CMD check: no new NOTEs/WARNINGs/ERRORs vs. the currentdevbranch (the one remaining ERROR, a missing@exportonpU(), is pre-existing and already tracked separately).No
.Rd/NAMESPACEchange needed (roxygen block unchanged).No
testthat/automated tests added, per repo convention.Generated by Claude Code