Skip to content

Conversation

@tonywu1999
Copy link
Contributor

@tonywu1999 tonywu1999 commented Aug 26, 2025

User description

Motivation and Context

Accidentally used arrows instead of equal signs in #171

Changes

  • Fix styling from arrows to equal signs.

Testing

Existing unit tests pass

Checklist Before Requesting a Review

  • I have read the MSstats contributing guidelines
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • I have run the devtools::document() command after my changes and committed the added files

PR Type

Enhancement, Tests


Description

  • Standardize assignment operators to '='

  • Align function usage across code, tests

  • Minor refactor in counts indexing


Diagram Walkthrough

flowchart LR
  utils["R/utils_groupcomparison.R getSamplesInfo()"] -- "style: <- to =" --> utils
  tests["inst/tinytest/test_utils_groupcomparison.R"] -- "style: <- to =" --> tests
  utils -- "consistent API expectations" --> tests
Loading

File Walkthrough

Relevant files
Enhancement
utils_groupcomparison.R
Use '=' for counts reassignment                                                   

R/utils_groupcomparison.R

  • Replace '<-' with '=' for assignment
  • Keep logic and data.table operations unchanged
+1/-1     
Tests
test_utils_groupcomparison.R
Tests updated to '=' assignment style                                       

inst/tinytest/test_utils_groupcomparison.R

  • Replace '<-' with '=' throughout tests
  • Preserve all test logic and expectations
+49/-49 

Summary by CodeRabbit

  • Style
    • Standardized assignment operator usage for consistency; no behavioral changes.
  • Tests
    • Updated test code to use consistent assignment syntax; test logic and outcomes unchanged.
  • Notes
    • No user-facing changes in this release.

@coderabbitai
Copy link

coderabbitai bot commented Aug 26, 2025

Walkthrough

Replaced R assignment operator <- with = in a utility function and corresponding tests. No logic, control flow, or public API changes.

Changes

Cohort / File(s) Summary
Utilities: assignment syntax
R/utils_groupcomparison.R
Switched assignment in .countMissingPercentage from <- to = with no behavioral change.
Tests: assignment syntax
inst/tinytest/test_utils_groupcomparison.R
Replaced <- with = across tests; test data and assertions unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Suggested labels

Review effort 2/5

Suggested reviewers

  • mstaniak

Poem

A whisker twitch, a tidy tweak,
I hop through code with syntax chic.
From <- to =, neat as can be,
Carrots aligned in harmony.
Tests still nibble, green and bright—
Thump! Another tidy night. 🥕🐇

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-arrows

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Reordering Bug

The new reindexing of counts using match(intersect(colnames(contrast_matrix), GROUP), GROUP) could introduce NA rows or drop groups unexpectedly if names are missing or ordering assumptions change. Verify behavior when contrast_matrix has groups not present in counts and ensure no NA indices slip through.

counts = counts[match(intersect(colnames(contrast_matrix), GROUP), GROUP), ]
Test Determinism

Tests now construct objects with = consistently, but some expectations depend on ordering derived from contrast_matrix and samples_info. Confirm tests cover mismatched ordering between contrast_matrix columns and summarized$GROUP to catch potential reindexing issues introduced in the code change.

expect_true(setequal(names(output), c(names(result), "MissingPercentage")), 
            info = "Basic functionality: No extraneous columns added")

## Test 2: With imputed values
contrast_matrix = matrix(c(1, -1), nrow = 1, ncol = 2)
colnames(contrast_matrix) = c("Group1", "Group2")
summarized = data.table::data.table(
    GROUP = c("Group1", "Group2"),
    TotalGroupMeasurements = c(100, 100),
    NumMeasuredFeature = c(80, 70),
    NumImputedFeature = c(10, 20)
)
result = list()
samples_info = data.table::data.table(GROUP = c("Group1", "Group2"), NumRuns = c(10, 10))
output = MSstats:::.countMissingPercentage(contrast_matrix, summarized, result, samples_info, TRUE)
expected_missing = 1 - (80 + 70) / (100 + 100) # 0.25
expected_imputed = (10 + 20) / (100 + 100) # 0.15
expect_equal(output$MissingPercentage[1], expected_missing, info = "Imputed values: Missing percentage calculation")
expect_equal(output$ImputationPercentage[1], expected_imputed, info = "Imputed values: Imputation percentage calculation")

## Test 3: With empty conditions (groups not in summarized data)
contrast_matrix = matrix(c(1, -1, 0), nrow = 1, ncol = 3)
colnames(contrast_matrix) = c("Group1", "Group2", "Group3")
summarized = data.table::data.table(
    GROUP = c("Group1"),
    TotalGroupMeasurements = c(100),
    NumMeasuredFeature = c(80),
    NumImputedFeature = c(0)
)

result = list()
samples_info = data.table::data.table(GROUP = c("Group1", "Group2", "Group3"), NumRuns = c(10, 10, 10))
output = MSstats:::.countMissingPercentage(contrast_matrix, summarized, result, samples_info, FALSE)
expect_equal(length(output$MissingPercentage), 1, info = "Empty conditions: MissingPercentage length")
expect_true(is.numeric(output$MissingPercentage), info = "Empty conditions: Numeric output")

## Test 4: Multiple contrasts with different missing patterns
contrast_matrix = matrix(c(1, -1, 0, 
                            0, 1, -1, 
                            1, 0, -1), nrow = 3, ncol = 3, byrow = TRUE)
colnames(contrast_matrix) = c("Group3", "Group2", "Group1")
summarized = data.table::data.table(
    GROUP = c("Group1", "Group2", "Group3"),
    TotalGroupMeasurements = c(100, 100, 100),
    NumMeasuredFeature = c(90, 80, 70),
    NumImputedFeature = c(5, 10, 15)
)
result = list()
samples_info = data.table::data.table(
    GROUP = c("Group3", "Group2", "Group1"), 
    NumRuns = c(1, 1, 1)
)
output = MSstats:::.countMissingPercentage(contrast_matrix, summarized, result, samples_info, TRUE)

expected_missing_1 = 1 - (70 + 80) / (100 + 100)  # 1 - 150/200 = 0.25
expected_imputed_1 = (15 + 10) / (100 + 100)      # 25/200 = 0.125
expected_missing_2 = 1 - (80 + 90) / (100 + 100)  # 1 - 170/200 = 0.15
expected_imputed_2 = (10 + 5) / (100 + 100)       # 15/200 = 0.075
expected_missing_3 = 1 - (70 + 90) / (100 + 100)  # 1 - 160/200 = 0.20
expected_imputed_3 = (15 + 5) / (100 + 100)       # 20/200 = 0.10

expect_equal(length(output$MissingPercentage), 3, info = "Column ordering: MissingPercentage length")
expect_equal(length(output$ImputationPercentage), 3, info = "Column ordering: ImputationPercentage length")
expect_equal(output$MissingPercentage[1], expected_missing_1, info = "Column ordering: Contrast 1 missing percentage (Group3 vs Group2)")

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against NA indices

match can return NA for non-matches, which will propagate NA rows and potentially
reorder incorrectly, leading to dropped or NA rows in counts. Filter to valid
indices before subsetting to avoid accidental NA subsetting and preserve intended
ordering.

R/utils_groupcomparison.R [409]

-counts = counts[match(intersect(colnames(contrast_matrix), GROUP), GROUP), ]
+ord <- match(intersect(colnames(contrast_matrix), counts$GROUP), counts$GROUP)
+ord <- ord[!is.na(ord)]
+counts <- counts[ord, ]
Suggestion importance[1-10]: 7

__

Why: Using match directly can introduce NA indices and unintended subsetting; filtering out NAs as suggested is correct and safer while preserving order. It's a moderate robustness improvement rather than a critical bug fix.

Medium
General
Prevent potential zero-division in test

Avoid expressions that could divide by zero in edge cases; if both totals are zero,
this would yield NaN and make the test brittle. Add a safeguard in the expectation
to handle zero denominators to keep the test robust.

inst/tinytest/test_utils_groupcomparison.R [113]

-expected_missing = 1 - (0 + 80) / (0 + 100) # 0.2
+denom <- (0 + 100)
+expected_missing = if (denom == 0) NA_real_ else 1 - (0 + 80) / denom # 0.2
Suggestion importance[1-10]: 4

__

Why: The test line is valid for the given values and improves only edge-case robustness if denominators could be zero; it's reasonable but minor and not directly required by the current PR changes.

Low

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
R/utils_groupcomparison.R (1)

409-409: Style-only change looks good; consider making GROUP column reference explicit to avoid NSE pitfalls.

The switch from <- to = is fine and consistent with the surrounding file style. Minor robustness nit: referencing GROUP as a bare symbol outside a data.table j/by context relies on the current environment. Making the column reference explicit improves clarity and avoids accidental name masking.

Apply this minimal, safer refactor:

-        counts = counts[match(intersect(colnames(contrast_matrix), GROUP), GROUP), ]
+        idx = match(intersect(colnames(contrast_matrix), counts$GROUP), counts$GROUP)
+        counts = counts[idx, ]
inst/tinytest/test_utils_groupcomparison.R (1)

83-84: Optional: add a test asserting column/order stability for repeated runs.

You already validate ordering with permuted column names. Consider one more assertion that output$MissingPercentage aligns with rownames(contrast_matrix) when there are repeated GROUP rows pre-aggregation, to guard against future refactors in the counting step.

If useful, I can draft a small additional test case that duplicates GROUP rows before aggregation and checks the order and values.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8c137ef and b6a6fee.

📒 Files selected for processing (2)
  • R/utils_groupcomparison.R (1 hunks)
  • inst/tinytest/test_utils_groupcomparison.R (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (2)
inst/tinytest/test_utils_groupcomparison.R (2)

3-6: LGTM on assignment operator normalization across tests.

All changes are purely stylistic (<-=) and preserve test intent and calculations. The suite continues to exercise ordering, empty-group handling, and imputation logic appropriately.

Also applies to: 12-13, 22-25, 35-42, 43-46, 52-54, 61-63, 68-72, 78-84, 102-104, 110-114, 117-131, 134-141


102-114: Edge-case assertion looks correct.

The “complete missing group” case correctly computes 0.2. No issues introduced by the assignment style change.

@tonywu1999 tonywu1999 merged commit b42eaff into devel Aug 26, 2025
3 checks passed
@tonywu1999 tonywu1999 deleted the fix-arrows branch August 26, 2025 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants