feat(getSubnetworkFromIndra): Add filter for including infinity fold change#80
feat(getSubnetworkFromIndra): Add filter for including infinity fold change#80tonywu1999 merged 3 commits intodevelfrom
Conversation
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/utils_getSubnetworkFromIndra.R (1)
158-184:⚠️ Potential issue | 🟡 MinorPotential issue: infinite values may be dropped by
logfc_cutofffilter before re-merging.The current logic extracts
infinite_fc_proteinsat line 160, but the subsequentlogfc_cutofffilter at line 171 usesabs(input$log2FC) > logfc_cutoff. Sinceabs(Inf) > logfc_cutoffevaluates toTRUE, infinite values would actually pass this filter. However, ifinclude_infinite_fc = FALSEandlogfc_cutoffis set, infinite values would still pass the logfc filter and remain in the result, which may not be the intended behavior.Consider explicitly excluding infinite values during the logfc_cutoff filtering when
include_infinite_fc = FALSE:Proposed fix
if (!is.null(logfc_cutoff)) { if (!is.numeric(logfc_cutoff) || length(logfc_cutoff) != 1 || logfc_cutoff < 0) { stop("logfc_cutoff must be a single positive numeric value") } - input <- input[!is.na(input$log2FC) & abs(input$log2FC) > logfc_cutoff, ] + input <- input[!is.na(input$log2FC) & is.finite(input$log2FC) & abs(input$log2FC) > logfc_cutoff, ] }This ensures infinite values are only included when explicitly requested via
include_infinite_fc = TRUE.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/utils_getSubnetworkFromIndra.R` around lines 158 - 184, The filter for logfc_cutoff currently lets infinite log2FC values pass unless include_infinite_fc is TRUE; update the logfc cutoff step in the function to explicitly exclude infinite values when include_infinite_fc is FALSE by changing the filter from input <- input[!is.na(input$log2FC) & abs(input$log2FC) > logfc_cutoff, ] to a conditional that adds !is.infinite(input$log2FC) when include_infinite_fc is FALSE (keep existing extraction of infinite_fc_proteins when include_infinite_fc is TRUE and continue to re-merge them only when requested).
🧹 Nitpick comments (1)
R/utils_getSubnetworkFromIndra.R (1)
174-184: Consider consolidating the merge operations for clarity.The two separate
rbind+ deduplication blocks could be combined into a single operation for better readability and maintainability. The current approach works correctly but the precedence order (exempt → infinite → filtered) could be made more explicit.Optional consolidation
- # Combine filtered data with exempt proteins and remove duplicates - if (!is.null(exempt_proteins) && nrow(exempt_proteins) > 0) { - combined_input <- rbind(exempt_proteins, input) - # Remove duplicates based on Protein column, keeping first occurrence - input <- combined_input[!duplicated(combined_input$Protein), ] - } - - if (!is.null(infinite_fc_proteins) && nrow(infinite_fc_proteins) > 0) { - combined_input <- rbind(infinite_fc_proteins, input) - input <- combined_input[!duplicated(combined_input$Protein), ] - } + # Combine filtered data with exempt and infinite proteins + # Priority order: exempt > infinite > filtered (first occurrence kept) + parts_to_merge <- list(exempt_proteins, infinite_fc_proteins, input) + parts_to_merge <- Filter(function(x) !is.null(x) && nrow(x) > 0, parts_to_merge) + if (length(parts_to_merge) > 1) { + combined_input <- do.call(rbind, parts_to_merge) + input <- combined_input[!duplicated(combined_input$Protein), ] + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/utils_getSubnetworkFromIndra.R` around lines 174 - 184, Consolidate the two rbind+dedup blocks by building a single stacked data.frame in the explicit precedence order (exempt_proteins first, then infinite_fc_proteins, then input) and then remove duplicates by Protein keeping the first occurrence; locate the logic using the variables exempt_proteins, infinite_fc_proteins, input and replace the two separate combined_input creations with one combined_input <- rbind(...) followed by the single deduplication step input <- combined_input[!duplicated(combined_input$Protein), ] to make precedence and intent clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@R/utils_getSubnetworkFromIndra.R`:
- Around line 158-184: The filter for logfc_cutoff currently lets infinite
log2FC values pass unless include_infinite_fc is TRUE; update the logfc cutoff
step in the function to explicitly exclude infinite values when
include_infinite_fc is FALSE by changing the filter from input <-
input[!is.na(input$log2FC) & abs(input$log2FC) > logfc_cutoff, ] to a
conditional that adds !is.infinite(input$log2FC) when include_infinite_fc is
FALSE (keep existing extraction of infinite_fc_proteins when include_infinite_fc
is TRUE and continue to re-merge them only when requested).
---
Nitpick comments:
In `@R/utils_getSubnetworkFromIndra.R`:
- Around line 174-184: Consolidate the two rbind+dedup blocks by building a
single stacked data.frame in the explicit precedence order (exempt_proteins
first, then infinite_fc_proteins, then input) and then remove duplicates by
Protein keeping the first occurrence; locate the logic using the variables
exempt_proteins, infinite_fc_proteins, input and replace the two separate
combined_input creations with one combined_input <- rbind(...) followed by the
single deduplication step input <-
combined_input[!duplicated(combined_input$Protein), ] to make precedence and
intent clearer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05f1e744-d301-40de-b149-1210e02160e4
📒 Files selected for processing (4)
R/getSubnetworkFromIndra.RR/utils_cytoscapeNetwork.RR/utils_getSubnetworkFromIndra.Rman/getSubnetworkFromIndra.Rd
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #80 +/- ##
==========================================
+ Coverage 78.66% 80.09% +1.42%
==========================================
Files 7 7
Lines 825 839 +14
==========================================
+ Hits 649 672 +23
+ Misses 176 167 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
R/utils_getSubnetworkFromIndra.R (1)
187-191: Consider adding test coverage for direction filtering.The Codecov report indicates missing coverage in this file. The
"up"and"down"direction branches should be tested to ensure the filtering behaves correctly for both directions, including edge cases with infinite log2FC values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/utils_getSubnetworkFromIndra.R` around lines 187 - 191, Add unit tests for the direction filtering logic in getSubnetworkFromIndra: create test cases that call getSubnetworkFromIndra (or the helper containing the shown block) with direction = "up" and direction = "down" and an input data.frame that includes NA, positive, negative, zero, Inf and -Inf values for the log2FC column to assert only the expected rows remain (positive and finite for "up", negative and finite for "down"), and include edge assertions that infinite values are excluded and NA rows are dropped; ensure tests cover both branches that use input[!is.na(input$log2FC) & input$log2FC > 0, ] and input[!is.na(input$log2FC) & input$log2FC < 0, ] so Codecov reflects the coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@R/utils_getSubnetworkFromIndra.R`:
- Around line 187-191: Add unit tests for the direction filtering logic in
getSubnetworkFromIndra: create test cases that call getSubnetworkFromIndra (or
the helper containing the shown block) with direction = "up" and direction =
"down" and an input data.frame that includes NA, positive, negative, zero, Inf
and -Inf values for the log2FC column to assert only the expected rows remain
(positive and finite for "up", negative and finite for "down"), and include edge
assertions that infinite values are excluded and NA rows are dropped; ensure
tests cover both branches that use input[!is.na(input$log2FC) & input$log2FC >
0, ] and input[!is.na(input$log2FC) & input$log2FC < 0, ] so Codecov reflects
the coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71cd13dd-32a0-4a81-be17-02c0508a80cd
📒 Files selected for processing (3)
R/getSubnetworkFromIndra.RR/utils_getSubnetworkFromIndra.Rman/getSubnetworkFromIndra.Rd
2 participants
Motivation and Context
This PR adds support for including proteins with infinite log fold change values in network construction from INDRA and adds directional filtering for regulation. Proteins with infinite logFC arise when they are observed in only one experimental condition (e.g., zero in one condition and non-zero in the other); previously these proteins could be excluded. The new options give users explicit control to include such proteins and to filter by regulation direction ("both", "up", "down") while preserving backward compatibility.
Solution Summary
Detailed Changes
R/getSubnetworkFromIndra.R
R/utils_getSubnetworkFromIndra.R
R/utils_cytoscapeNetwork.R
man/getSubnetworkFromIndra.Rd
Minor: Roxygen and function documentation updated to reflect new parameters.
Unit Tests