refactor(getSubnetworkFromIndra): deprecate force_include_proteins parameter in favor of force_include_other#60
Conversation
…rameter in favor of force_include_other
WalkthroughThe PR updates package metadata and refactors getSubnetworkFromIndra: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant getSub as getSubnetworkFromIndra
participant Filter as .filterGetSubnetworkFromIndraInput
participant API as .callIndraCogexApi
participant Nodes as NodeConstruction
User->>getSub: call(input, statement_types=NULL, force_include_other=...)
getSub->>Filter: validate input, derive exemptions (HGNC ids)
Filter-->>getSub: filtered input (exemptions applied)
getSub->>API: request subnetwork (passes force_include_other)
API-->>getSub: edges & nodes
getSub->>Nodes: ensure endpoints present — add missing endpoints with defaults
Nodes-->>User: return complete nodes + edges
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ 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 |
|
Failed to generate code suggestions for PR |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #60 +/- ##
==========================================
- Coverage 64.30% 63.89% -0.42%
==========================================
Files 7 7
Lines 1216 1227 +11
==========================================
+ Hits 782 784 +2
- Misses 434 443 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
DESCRIPTION(1 hunks)R/getSubnetworkFromIndra.R(2 hunks)R/utils_getSubnetworkFromIndra.R(2 hunks)man/getSubnetworkFromIndra.Rd(2 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). (3)
- GitHub Check: test-coverage
- GitHub Check: pkgdown
- GitHub Check: build
🔇 Additional comments (8)
DESCRIPTION (1)
42-42: LGTM!Standard RoxygenNote version bump from documentation regeneration.
R/utils_getSubnetworkFromIndra.R (3)
9-31: LGTM!The validation logic correctly handles the renamed
force_include_otherparameter and includes it in the protein count validation.
41-67: LGTM!The API call correctly processes
force_include_otheridentifiers with proper format validation and helpful error messages.
109-159: LGTM!The exemption logic correctly extracts HGNC identifiers from
force_include_otherand handles the case where non-HGNC identifiers are provided (which won't match input data).man/getSubnetworkFromIndra.Rd (2)
11-11: LGTM!Default value for
statement_typescorrectly updated to NULL, consistent with the code changes.
54-56: LGTM!Documentation for
force_include_otherclearly describes the expected format with helpful examples.R/getSubnetworkFromIndra.R (2)
17-17: LGTM!Documentation correctly updated to reflect the new NULL default for
statement_types.
47-72: LGTM!Function signature and internal calls correctly updated to use
force_include_otherinstead of the deprecatedforce_include_proteinsparameter. All downstream function calls properly pass the renamed parameter.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
R/utils_getSubnetworkFromIndra.R (1)
105-105: Update parameter documentation to reflect broader identifier support.The documentation still states "list of proteins" but
force_include_otheraccepts any identifier format (e.g.,CHEBI:4911,HGNC:1234). This inconsistency was previously flagged and remains unaddressed.Apply this diff:
-#' @param force_include_other list of proteins to exempt from filtering +#' @param force_include_other character vector of identifiers to exempt from filtering
🧹 Nitpick comments (1)
R/utils_getSubnetworkFromIndra.R (1)
297-308: Critical bug fixed, but consider renaming for clarity.The undefined variable issue (
missing_nodes) flagged in previous reviews has been correctly resolved—line 307 now properly referencesextra_nodes. The logic ensures all edge endpoints appear in the final nodes data frame.However, the variable name
extra_force_include_otheris misleading, as these are any edge endpoints not present in the initial nodes set, not necessarily from theforce_include_otherparameter. Consider the optional refactor suggested in the previous review to rename it tomissing_node_idsfor clarity.Optional diff to improve naming clarity:
- extra_force_include_other <- setdiff(unique(c(edges$source, edges$target)), nodes$id) - if (length(extra_force_include_other) > 0) { + missing_node_ids <- setdiff(unique(c(edges$source, edges$target)), nodes$id) + if (length(missing_node_ids) > 0) { extra_nodes <- data.frame( - id = extra_force_include_other, + id = missing_node_ids, hgncName = NA, Site = NA, logFC = 0, adj.pvalue = 1, stringsAsFactors = FALSE ) nodes <- rbind(nodes, extra_nodes) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
R/utils_getSubnetworkFromIndra.R(2 hunks)
🔇 Additional comments (1)
R/utils_getSubnetworkFromIndra.R (1)
114-124: Exempt protein extraction logic is correct.The logic correctly handles HGNC-prefixed identifiers in
force_include_otherby extracting and matching them against the input data. Non-HGNC identifiers (e.g.,CHEBI:4911) are intentionally not included inexempt_proteinssince the input contains only protein data withHgncId. These non-protein identifiers are still passed to the INDRA API (lines 47-55) and will appear in the final network when they occur as edge endpoints (handled by lines 297-308).
2 participants
Summary by CodeRabbit
Chores
Refactor
Documentation