Skip to content

Devel#12

Closed
EmanuelSoda wants to merge 19 commits intomasterfrom
devel
Closed

Devel#12
EmanuelSoda wants to merge 19 commits intomasterfrom
devel

Conversation

@EmanuelSoda
Copy link
Owner

@EmanuelSoda EmanuelSoda commented Feb 8, 2026

Summary by Sourcery

Update plotting tests and z-score hit computation to be compatible with current ggplot2 and dplyr behavior, and adjust examples and tests accordingly.

Enhancements:

  • Refine find_zscore_hit to use dplyr::reframe for counting barcodes per gene, aligning with modern dplyr semantics.

Documentation:

  • Wrap the find_zscore_hit usage example in \dontrun{} to avoid execution during example checks.

Tests:

  • Update visualization, mapped-reads, barcode, and MDS tests to assert ggplot outputs using ggplot2::is_ggplot()/inherits instead of direct class string comparisons.
  • Comment out the robust z-score hit median test that no longer matches the updated implementation.

@sourcery-ai
Copy link

sourcery-ai bot commented Feb 8, 2026

Reviewer's Guide

This PR modernizes ggplot-related tests, updates z-score hit computation to use dplyr::reframe instead of summarise, and comments out a fragile robust z-score test, while lightly adjusting documentation for find_zscore_hit.

Flow diagram for updated find_zscore_hit z-score hit computation

flowchart TD
    A["find_zscore_hit(table_treate_vs_control, number_barcode, metric)"] --> B{metric}
    B -- "median" --> C["Group rows by Gene"]
    C --> D["Filter rows with Zscore < median(Zscore)"]
    D --> E["dplyr::group_by(Gene)"]
    E --> F["dplyr::reframe(numberOfBarcode = n())"]
    F --> G["Filter genes with numberOfBarcode > number_barcode"]
    G --> H["Return filtered table (median-based hits)"]

    B -- "mean" --> I["Group rows by Gene"]
    I --> J["Filter rows with Zscore < mean(Zscore)"]
    J --> K["dplyr::group_by(Gene)"]
    K --> L["dplyr::reframe(numberOfBarcode = n())"]
    L --> M["Filter genes with numberOfBarcode > number_barcode"]
    M --> N["Return filtered table (mean-based hits)"]
Loading

File-Level Changes

Change Details Files
Modernize visualization and MDS tests to assert ggplot objects using ggplot2 helpers instead of raw class inspection.
  • Replace expectations like expect_equal(class(x)[1], "gg") or class(plot)[2] == "ggplot" with expect_true(ggplot2::is_ggplot(...)) or expect_true(inherits(..., "ggplot")) to check for ggplot outputs more robustly.
  • Normalize plot variable naming by renaming local variables from p to plot where updated expectations reference the object.
  • Apply these updates consistently across visualization tests (trend, boxplot/violin, barcode plots, z-score distribution, mapped reads, barcode lost, common hit) and MDS/variance plotting tests.
tests/testthat/test-visualization.R
tests/testthat/test-MDS.R
Adjust find_zscore_hit implementation to use dplyr::reframe for counting barcodes per gene instead of summarise, and protect its example from running on CRAN/test environments.
  • Wrap the find_zscore_hit documentation example code in a \dontrun{} block so it is not executed during example runs.
  • Replace dplyr::summarise(numberOfBarcode = n()) with dplyr::reframe(numberOfBarcode = n()) in the median-branch to compute barcode counts per gene while returning an ungrouped tibble.
  • Replace dplyr::summarise(.data$numberOfBarcode) with dplyr::reframe(numberOfBarcode = n()) in the mean-branch, mirroring the new counting behavior and avoiding partial argument matching on summarise.
R/find_zscore_hit.R
Temporarily disable a robust z-score hit test that likely no longer matches the implementation or is unstable.
  • Comment out the entire find_robust_zscore_hit median test block to prevent it from running while keeping the code for future reference.
tests/testthat/test-statistics.R
Non-functional project metadata / vignette touch-ups (no concrete diff provided).
  • Update DESCRIPTION and Analysis_Example vignette, likely for housekeeping or compatibility, though no specific content changes are shown in the snippet.
DESCRIPTION
vignettes/Analysis_Example.Rmd

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • In find_zscore_hit(), the switch from summarise() to reframe() changes the required dplyr version and grouping behavior; if the intention is to keep the same grouped output structure as before, consider explicitly checking/setting grouping or stick with summarise() for compatibility.
  • The find_robust_zscore_hit median test in test-statistics.R is now fully commented out; if it is intentionally deprecated it may be clearer to remove it entirely or add a brief comment explaining why it is disabled.
  • The tests now mix ggplot2::is_ggplot() and inherits(..., "ggplot") for plot checks; consider standardizing on one approach across the test suite for consistency.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `find_zscore_hit()`, the switch from `summarise()` to `reframe()` changes the required dplyr version and grouping behavior; if the intention is to keep the same grouped output structure as before, consider explicitly checking/setting grouping or stick with `summarise()` for compatibility.
- The `find_robust_zscore_hit median` test in `test-statistics.R` is now fully commented out; if it is intentionally deprecated it may be clearer to remove it entirely or add a brief comment explaining why it is disabled.
- The tests now mix `ggplot2::is_ggplot()` and `inherits(..., "ggplot")` for plot checks; consider standardizing on one approach across the test suite for consistency.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@EmanuelSoda EmanuelSoda closed this Feb 8, 2026
@EmanuelSoda EmanuelSoda reopened this Feb 8, 2026
@EmanuelSoda EmanuelSoda closed this Feb 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants