Fix fraction efficient#126
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactored fraction tie-resolution logic in MSstatsConvert by replacing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
inst/tinytest/test_fractions.R (1)
71-79: Add a three-fraction regression for tie resolution.These checks only exercise two-fraction cases. Please add one where two fractions tie on
n_obsand a third has fewer observations but a higher mean intensity; that’s the edge case the new resolver can currently pick incorrectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inst/tinytest/test_fractions.R` around lines 71 - 79, Add a new unit test that covers the three-fraction edge case for MSstatsConvert:::.removeOverlappingFeatures: construct a fractionated dataset for a new feature (e.g., feature "C") with three fraction groups where two fractions tie on n_obs and the third has fewer observations but a higher mean intensity, call .removeOverlappingFeatures(fractionated[feature == "C"]) and assert that unique(...$Fraction) is the expected fraction (the resolver should prefer the fraction among the tied n_obs with the higher mean intensity); reference the existing test pattern using fractionated, .removeOverlappingFeatures, and $Fraction to mirror the other expect_equal checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/utils_fractions.R`:
- Around line 298-303: avg_abundance currently computes mean(Intensity) across
all fractions for tie_features which can let a fraction with higher mean but
fewer observations win; update the logic to compute n_obs per (feature,
Fraction) alongside mean_abundance, then for each feature restrict candidates to
only those Fraction rows where n_obs equals the per-feature maximum n_obs (i.e.,
keep only fractions that tied for the top observation count) before selecting
the best by mean_abundance; adjust the code that builds avg_abundance and
best_tied (referenced symbols: avg_abundance, best_tied, tie_features, Fraction,
Intensity) so best_tied = avg_abundance[ n_obs == max(n_obs) ,
.SD[which.max(mean_abundance)], by = "feature"] (or equivalent filtering) to
ensure tie-breaking only occurs among the max-n_obs fractions.
---
Nitpick comments:
In `@inst/tinytest/test_fractions.R`:
- Around line 71-79: Add a new unit test that covers the three-fraction edge
case for MSstatsConvert:::.removeOverlappingFeatures: construct a fractionated
dataset for a new feature (e.g., feature "C") with three fraction groups where
two fractions tie on n_obs and the third has fewer observations but a higher
mean intensity, call .removeOverlappingFeatures(fractionated[feature == "C"])
and assert that unique(...$Fraction) is the expected fraction (the resolver
should prefer the fraction among the tied n_obs with the higher mean intensity);
reference the existing test pattern using fractionated,
.removeOverlappingFeatures, and $Fraction to mirror the other expect_equal
checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccb74428-a55b-44bb-9041-751ffab5adea
📒 Files selected for processing (2)
R/utils_fractions.Rinst/tinytest/test_fractions.R
1 participant
Motivation and Context
Please include relevant motivation and context of the problem along with a short summary of the solution.
Changes
Please provide a detailed bullet point list of your changes.
Testing
Please describe any unit tests you added or modified to verify your changes.
Checklist Before Requesting a Review
Motivation and Context
The MSstatsConvert package processes mass spectrometry-based proteomics data and handles cases where peptide features are measured across multiple fractions. When a feature appears in multiple fractions within a sample, the code must select which fraction's measurement to retain. The previous implementation used a less efficient approach that lacked clear separation of concerns between fraction selection and tie-resolution logic. This PR improves efficiency and code clarity by refactoring the fraction selection mechanism.
Solution
The refactoring centralizes tie-resolution logic into a dedicated function and makes the fraction selection process more efficient. Instead of applying tie-break logic during iteration, the new approach:
Detailed Changes
R/utils_fractions.RRefactored
.removeOverlappingFeatures():uniqueN(Run)among rows with!is.na(Intensity) & Intensity > 0per feature-Fraction combinationfraction_mapvia.resolveFractionTies()helper functioninput[fraction_map, on=.(feature, Fraction), nomatch=0]fraction_keepcolumn logic that previously relied on row-by-row evaluationReplaced
.getCorrectFraction()with.resolveFractionTies(input, max_fractions):tie_featureswhere multiple fractions share maximumn_obsper featurewhich.max(mean_abundance)grouped by feature and Fractionrbind()combining both groupsLines changed: +35/-24
inst/tinytest/test_fractions.RUpdated test logic: Instead of calling
.getCorrectFraction()directly on individual features, tests now:.removeOverlappingFeatures()on per-feature subsetsFractionvaluesTest coverage maintained: The test continues to validate
.removeOverlappingFeatures()on the full dataset usingexpect_identical()Lines changed: +9/-5
Unit Tests
The test suite verifies the refactored logic through:
All tests continue to pass with the new implementation, confirming backward compatibility of the selection behavior.
Coding Guidelines
No coding guideline violations identified. The code:
.resolveFractionTies()function:=,.N,.SD,by=,on=)