Skip to content

Fix getRows/getCols filtering and reporting; bump version; add audit - #37

Merged
vertesy merged 3 commits into
devfrom
codex/audit-and-improve-codeandroll2-repository
Aug 27, 2026
Merged

Fix getRows/getCols filtering and reporting; bump version; add audit#37
vertesy merged 3 commits into
devfrom
codex/audit-and-improve-codeandroll2-repository

Conversation

@vertesy

@vertesy vertesy commented Aug 27, 2026

Copy link
Copy Markdown
Owner

Motivation

  • Fix incorrect and failing behavior in row/column selection helpers where remove0only = TRUE referenced an undefined mx and could error in clean sessions.
  • Correct reporting of missing requested names which used the set-difference in the wrong direction and did not reflect the requested-but-missing items.
  • Add compact input assertions and record an audit to document findings and deferred items before further, larger changes.

Description

  • Replace mx usage in getRows() and getCols() with localized selected_mat <- mat[... , drop = FALSE] and filter rows/columns from that selection to avoid relying on an external object.
  • Add compact stopifnot(...) input assertions to getRows() and getCols() to validate key arguments and preserve dimensions by returning with drop = FALSE.
  • Bump package version from 2.8.0 to 2.8.1 in Development/config.R and DESCRIPTION and add Development/Audit_20260827.md with a checklist of audit findings and deferred fixes.

Testing

  • Static regression assertions (Python checks that scan for the bad patterns and version updates) passed.
  • Structural check git diff --check (pre-commit style) reported no problems.
  • R CMD check and lintr::lint_package() were attempted but could not run in this environment because R / Rscript are not installed, so package checks and linter runs are pending in an R-equipped environment.

Codex Task

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bce0609de3

ℹ️ 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".

Comment thread R/CodeAndRoll2.R Outdated
#' @export
getRows <- function(mat, rownamez, silent = FALSE, removeNAonly = FALSE, remove0only = FALSE) {
stopifnot(
is.matrix(mat) || is.data.frame(mat), !is.null(rownames(mat)),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Allow sparse matrix classes in the input check

Problem: getRows() now rejects S4 matrix implementations such as Matrix::dgCMatrix; the identical assertion in getCols() does the same. These objects are not base matrices or data frames, although the existing name lookup, subsetting, and row/column summary operations support them. Trigger: Any downstream caller passes a sparse matrix, including common Seurat workflows. Fix: Validate the required two-dimensional and named interface, or explicitly accept compatible Matrix classes, instead of restricting mat to base matrices and data frames.

AGENTS.md reference: AGENTS.md:L46-L50

Useful? React with 👍 / 👎.

Comment thread R/CodeAndRoll2.R Outdated
if (!silent) {
iprint(length(idx), "/", length(rownamez), "are found. Missing: ", length(setdiff(row.names(mat), rownamez)))
missing_rows <- setdiff(rownamez, row.names(mat))
iprint(length(idx), "/", length(rownamez), "are found. Missing: ", length(missing_rows))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Count matched names before applying value filters

Problem: length(idx) counts rows retained after zero/NA filtering, but the message describes that number as rows that “are found.” Trigger: If a requested row exists but is removed by remove0only or removeNAonly, getRows() can print 0 / 1 are found. Missing: 0; getCols() has the same issue. Fix: Capture the matched-name count immediately after intersect(), or report the matched and retained counts separately.

Useful? React with 👍 / 👎.

@vertesy

vertesy commented Aug 27, 2026

Copy link
Copy Markdown
Owner Author

@copilot

Please validate and resolve problems:

1
"Allow sparse matrix classes in the input check
Problem: getRows() now rejects S4 matrix implementations such as Matrix::dgCMatrix; the identical assertion in getCols() does the same. These objects are not base matrices or data frames, although the existing name lookup, subsetting, and row/column summary operations support them. Trigger: Any downstream caller passes a sparse matrix, including common Seurat workflows. Fix: Validate the required two-dimensional and named interface, or explicitly accept compatible Matrix classes, instead of restricting mat to base matrices and data frames."

2
"P2 Badge Count matched names before applying value filters
Problem: length(idx) counts rows retained after zero/NA filtering, but the message describes that number as rows that “are found.” Trigger: If a requested row exists but is removed by remove0only or removeNAonly, getRows() can print 0 / 1 are found. Missing: 0; getCols() has the same issue. Fix: Capture the matched-name count immediately after intersect(), or report the matched and retained counts separately."

Thank you!

Co-authored-by: vertesy <5101911+vertesy@users.noreply.github.com>

Copilot AI commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

@copilot

Please validate and resolve problems:
...

Addressed in fda93f8.

  • Updated getRows() / getCols() input checks to also accept Matrix-compatible S4 inputs (methods::is(mat, "Matrix")) while keeping required row/column name checks.
  • Fixed badge/reporting semantics by capturing matched-name count immediately after intersect() and reporting both matched and retained counts after optional NA/zero filters.

@vertesy
vertesy merged commit 620fda6 into dev Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants