Filter by PTM site match#68
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces the INDRA api_key-based curation step with a PTM-site filtering option and modular helper filters (.filterByPtmSite, .filterByCuration); also exposes node font-size options in HTML/Cytoscape visualization APIs and updates documentation accordingly. Changes
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 |
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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)
291-294:⚠️ Potential issue | 🟠 MajorUse
bigint_as_char = TRUEinfromJSON()to preserve integer precision formatches_hash.
matches_hashin INDRA statement JSON is a signed 64-bit integer, which can reach up to ~19 digits. The current code at lines 291–293 callsfromJSON()without thebigint_as_charparameter, causing large integers to be deserialized into Rnumeric(double) type. Since doubles can only represent ~15–16 significant decimal digits accurately, hashes exceeding 2^53 will lose precision and become corrupted. Whenas.character(stmt_hash)is called at line 73 to build the curation API URL, the rounded value is used, potentially querying an incorrect curation list.Pass
bigint_as_char = TRUEtofromJSON()to keep large integers as character strings:Suggested fix
stmt_json <- fromJSON(query(res, x)$data$stmt_json, bigint_as_char = 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 291 - 294, The deserialization of INDRA statement JSON loses precision for 64-bit integer matches_hash because fromJSON(...) is called without bigint_as_char; update the call in the stmt_hash construction (where stmt_json <- fromJSON(query(res, x)$data$stmt_json) is used) to pass bigint_as_char = TRUE so matches_hash remains a character (preserving full precision) and downstream uses such as as.character(stmt_hash) produce the correct hash string.
♻️ Duplicate comments (2)
man/generateCytoscapeConfig.Rd (1)
13-14: Missing\item{node_font_size}in\arguments— same asexportNetworkToHTML.Rd.See the comment on
man/exportNetworkToHTML.Rdline 12.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@man/generateCytoscapeConfig.Rd` around lines 13 - 14, The \arguments section of man/generateCytoscapeConfig.Rd is missing an entry for the node_font_size argument; add a \item{node_font_size}{...} description matching the one used in man/exportNetworkToHTML.Rd (explain it’s the default node font size, units and default value 12) so the documentation lists node_font_size alongside layout_options and other args.man/previewNetworkInBrowser.Rd (1)
7-13: Missing\item{nodeFontSize}in\arguments— same asexportNetworkToHTML.Rd.See the comment on
man/exportNetworkToHTML.Rdline 12.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@man/previewNetworkInBrowser.Rd` around lines 7 - 13, The \arguments section of previewNetworkInBrowser.Rd is missing documentation for the nodeFontSize parameter; add an \item{nodeFontSize}{...} entry describing its purpose and default (same wording and style as the nodeFontSize item in exportNetworkToHTML.Rd) so the help page documents the nodeFontSize argument for the previewNetworkInBrowser function.
🧹 Nitpick comments (1)
R/utils_getSubnetworkFromIndra.R (1)
363-365:.filterByCurationand.filterByPtmSitelack roxygen documentation blocks.Every other internal helper in this file (
.validateGetSubnetworkFromIndraInput,.callIndraCogexApi,.filterIndraResponse, etc.) has@title,@param,@return,@keywords internal, and@noRdtags. These two new helpers have none, making them inconsistent and invisible to package documentation tooling.Also applies to: 377-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/utils_getSubnetworkFromIndra.R` around lines 363 - 365, Add roxygen3 documentation blocks to the two internal helpers `.filterByCuration` and `.filterByPtmSite` so they match the other helpers: include `@title` (short description), `@param` entries for each function argument (e.g., nodes, edges, filter_by_curation for `.filterByCuration`; nodes, edges, filter_by_ptm_site for `.filterByPtmSite`), `@return` describing the returned object (e.g., filtered edges/nodes or logical vector), and the tags `@keywords` internal and `@noRd`; place the block immediately above each function definition so package documentation tooling treats them as internal and consistent with `.validateGetSubnetworkFromIndraInput`, `.callIndraCogexApi`, and `.filterIndraResponse`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@man/exportNetworkToHTML.Rd`:
- Line 12: Add a missing \item entry for the nodeFontSize parameter in the
\arguments section of man/exportNetworkToHTML.Rd: document the parameter name
(nodeFontSize), its purpose (controls node label/font size in the exported
HTML), and its default (12). Do the same for the other RD files mentioned: add
an \item for node_font_size in man/previewNetworkInBrowser.Rd and
man/generateCytoscapeConfig.Rd with a short description and the correct default
value, ensuring the documented names match the function arguments (nodeFontSize
/ node_font_size) so R CMD check no longer reports undocumented parameters.
In `@R/utils_getSubnetworkFromIndra.R`:
- Around line 377-385: In .filterByPtmSite: when subsetting edges using
ptm_overlap[paste(edges$source, edges$target, edges$interaction, sep = "-")] NAs
propagate and produce NA rows; fix by computing the composite key
(paste(edges$source, edges$target, edges$interaction, sep = "-")), look up
ptm_overlap for that key into a vector (e.g., sel), coerce missing entries to a
safe value (e.g., replace NA with "" or use !is.na(sel) combined with sel !=
""), and then subset edges by sel != "" (or equivalent logical mask) before
proceeding to !is.na(edges$site) so no NA logicals cause NA rows.
- Around line 363-375: The function .filterByCuration refers to
evidence_count_cutoff which is out of scope and causes a runtime error; add
evidence_count_cutoff as an explicit parameter to .filterByCuration (e.g.,
.filterByCuration = function(nodes, edges, filter_by_curation,
evidence_count_cutoff)) and use that parameter where edges are filtered, then
update every call-site (notably the call in getSubnetworkFromIndra /
R/getSubnetworkFromIndra.R) to pass the appropriate evidence_count_cutoff value
from the parent scope; ensure the function signature and all invocations are
consistent so the cutoff is threaded through.
- Line 379: The call to calculatePTMOverlapAggregated from
utils_getSubnetworkFromIndra.R fails because the function in
R/visualizeNetworksWithHTML.R is not exported; add an `@export` roxygen2 tag
immediately above the calculatePTMOverlapAggregated function definition in
R/visualizeNetworksWithHTML.R (or alternatively add an
export(calculatePTMOverlapAggregated) entry to the NAMESPACE) so R CMD check and
cross-file calls can find the function.
---
Outside diff comments:
In `@R/utils_getSubnetworkFromIndra.R`:
- Around line 291-294: The deserialization of INDRA statement JSON loses
precision for 64-bit integer matches_hash because fromJSON(...) is called
without bigint_as_char; update the call in the stmt_hash construction (where
stmt_json <- fromJSON(query(res, x)$data$stmt_json) is used) to pass
bigint_as_char = TRUE so matches_hash remains a character (preserving full
precision) and downstream uses such as as.character(stmt_hash) produce the
correct hash string.
---
Duplicate comments:
In `@man/generateCytoscapeConfig.Rd`:
- Around line 13-14: The \arguments section of man/generateCytoscapeConfig.Rd is
missing an entry for the node_font_size argument; add a
\item{node_font_size}{...} description matching the one used in
man/exportNetworkToHTML.Rd (explain it’s the default node font size, units and
default value 12) so the documentation lists node_font_size alongside
layout_options and other args.
In `@man/previewNetworkInBrowser.Rd`:
- Around line 7-13: The \arguments section of previewNetworkInBrowser.Rd is
missing documentation for the nodeFontSize parameter; add an
\item{nodeFontSize}{...} entry describing its purpose and default (same wording
and style as the nodeFontSize item in exportNetworkToHTML.Rd) so the help page
documents the nodeFontSize argument for the previewNetworkInBrowser function.
---
Nitpick comments:
In `@R/utils_getSubnetworkFromIndra.R`:
- Around line 363-365: Add roxygen3 documentation blocks to the two internal
helpers `.filterByCuration` and `.filterByPtmSite` so they match the other
helpers: include `@title` (short description), `@param` entries for each function
argument (e.g., nodes, edges, filter_by_curation for `.filterByCuration`; nodes,
edges, filter_by_ptm_site for `.filterByPtmSite`), `@return` describing the
returned object (e.g., filtered edges/nodes or logical vector), and the tags
`@keywords` internal and `@noRd`; place the block immediately above each function
definition so package documentation tooling treats them as internal and
consistent with `.validateGetSubnetworkFromIndraInput`, `.callIndraCogexApi`,
and `.filterIndraResponse`.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
R/getSubnetworkFromIndra.RR/utils_getSubnetworkFromIndra.Rman/exportNetworkToHTML.Rdman/generateCytoscapeConfig.Rdman/getSubnetworkFromIndra.Rdman/previewNetworkInBrowser.Rd
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
R/visualizeNetworksWithHTML.R (3)
379-403:⚠️ Potential issue | 🟡 MinorMissing
@param node_font_sizeingenerateCytoscapeConfigroxygen block.The
node_font_size = 12argument is present in the function signature (Line 403) but has no corresponding@paramtag in the documentation block (Lines 379–397). Regenerating docs from roxygen will leaveman/generateCytoscapeConfig.Rdwithout this entry, andR CMD checkwill flag it as an undocumented argument.📝 Proposed fix
#' `@param` layout_options Optional list of layout configuration options +#' `@param` node_font_size Font size for node labels in pixels (default: 12) #' `@export`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/visualizeNetworksWithHTML.R` around lines 379 - 403, Add a `@param` tag for node_font_size to the roxygen block above the generateCytoscapeConfig function: document that node_font_size is the numeric font size for node labels (default = 12), place it alongside the existing `@param` entries (e.g., after display_label_type or container_id), and then regenerate the package documentation so man/generateCytoscapeConfig.Rd includes this parameter.
1149-1153:⚠️ Potential issue | 🟡 MinorNo input validation for
nodeFontSize.Non-numeric or negative values (e.g.,
nodeFontSize = "large"ornodeFontSize = -5) pass through topaste0(node_font_size, "px")at Line 440, producing invalid CSS that is silently ignored by the browser. A lightweight guard at the entry point would surface the error early.🛡️ Proposed fix
exportNetworkToHTML <- function(nodes, edges, filename = "network_visualization.html", displayLabelType = "id", nodeFontSize = 12, ...) { + if (!is.numeric(nodeFontSize) || nodeFontSize <= 0) { + stop("`nodeFontSize` must be a positive number.") + }Apply the same guard at the top of
previewNetworkInBrowserandgenerateCytoscapeConfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/visualizeNetworksWithHTML.R` around lines 1149 - 1153, The functions exportNetworkToHTML, previewNetworkInBrowser, and generateCytoscapeConfig do not validate the nodeFontSize parameter, allowing non-numeric or negative values to reach paste0(node_font_size, "px") (see node_font_size usage) and produce invalid CSS; add a lightweight guard at the start of each function that checks nodeFontSize is numeric (or coercible) and greater than zero, coerce or convert to a numeric value if safe, and otherwise stop/throw a clear error message indicating nodeFontSize must be a positive number so invalid inputs are rejected early.
1170-1182:⚠️ Potential issue | 🟡 MinorMissing
@param nodeFontSizeinpreviewNetworkInBrowserroxygen block.
nodeFontSize = 12is in the function signature (Line 1181) but absent from the documentation block (Lines 1170–1178). This will produce anR CMD checkundocumented-argument warning and leaveman/previewNetworkInBrowser.Rdincomplete after re-generation.📝 Proposed fix
#' `@param` displayLabelType Type of label to display ("id" or "hgncName") +#' `@param` nodeFontSize Font size for node labels (default: 12) #' `@param` ... Additional arguments passed to exportCytoscapeToHTML()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/visualizeNetworksWithHTML.R` around lines 1170 - 1182, The roxygen block for previewNetworkInBrowser is missing a `@param` entry for nodeFontSize; add a line like "@param nodeFontSize Numeric font size for node labels (default 12)." to the documentation block above the previewNetworkInBrowser function so R CMD check won't warn and the generated man/previewNetworkInBrowser.Rd includes this argument.
♻️ Duplicate comments (1)
man/exportNetworkToHTML.Rd (1)
12-12: LGTM – resolves the previously flagged undocumented-parameter warning.The
nodeFontSize = 12entry is now present in both\usageand\arguments, bringing this.Rdfile into R CMD check compliance.Also applies to: 25-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@man/exportNetworkToHTML.Rd` at line 12, Add the missing documentation for the nodeFontSize parameter in the Rd file: ensure the symbol nodeFontSize = 12 appears in both the \usage and \arguments sections of exportNetworkToHTML.Rd and add a matching descriptive entry for nodeFontSize in the \arguments block (and mirror the same addition for the other duplicate occurrence referenced), so the parameter is documented consistently and R CMD check no longer flags an undocumented parameter.
🧹 Nitpick comments (1)
R/getSubnetworkFromIndra.R (1)
35-37: Clarify expected PTM-site input fields.Consider listing the required PTM-site column names/format (or linking to the specific input schema) so users can enable this filter without guesswork.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/getSubnetworkFromIndra.R` around lines 35 - 37, The documentation for the filter_by_ptm_site parameter is missing the exact expected PTM-site column names/format; update the Roxygen for the getSubnetworkFromIndra function to list the required input columns (e.g., "gene", "site", "residue" or whatever the package expects) or add a link to the input schema, and state the expected format (e.g., "S123" or "S123_phospho"); explicitly mention the column names used by the function (those parsed in getSubnetworkFromIndra or its helper that checks PTM sites) so users know how to structure their differential PTM abundance input before setting filter_by_ptm_site = TRUE.
🤖 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/getSubnetworkFromIndra.R`:
- Around line 62-63: The function getSubnetworkFromIndra currently accepts
filter_by_curation and filter_by_ptm_site and may receive a string if callers
used positional args after api_key was removed; add a fast guard at the start of
getSubnetworkFromIndra that validates filter_by_curation and filter_by_ptm_site
are logical (is.logical and length 1) and error with a clear message if they are
not (e.g., detect character inputs and tell callers to use named arguments or
restore api_key); alternatively implement a short deprecation shim that accepts
a first positional character argument as the removed api_key and emits a
deprecation error guiding callers to switch to named params—reference the
function name getSubnetworkFromIndra and the parameters filter_by_curation and
filter_by_ptm_site in the check.
- Line 67: Update the parameter name in the .filterIndraResponse function to
match the caller: rename the formal argument interaction_types to
statement_types so the call res <- .filterIndraResponse(res, statement_types,
evidence_count_cutoff, sources_filter) is explicit and clear; adjust any
internal references inside .filterIndraResponse (and its documentation/roxygen
if present) from interaction_types to statement_types to avoid breaking
behavior.
---
Outside diff comments:
In `@R/visualizeNetworksWithHTML.R`:
- Around line 379-403: Add a `@param` tag for node_font_size to the roxygen block
above the generateCytoscapeConfig function: document that node_font_size is the
numeric font size for node labels (default = 12), place it alongside the
existing `@param` entries (e.g., after display_label_type or container_id), and
then regenerate the package documentation so man/generateCytoscapeConfig.Rd
includes this parameter.
- Around line 1149-1153: The functions exportNetworkToHTML,
previewNetworkInBrowser, and generateCytoscapeConfig do not validate the
nodeFontSize parameter, allowing non-numeric or negative values to reach
paste0(node_font_size, "px") (see node_font_size usage) and produce invalid CSS;
add a lightweight guard at the start of each function that checks nodeFontSize
is numeric (or coercible) and greater than zero, coerce or convert to a numeric
value if safe, and otherwise stop/throw a clear error message indicating
nodeFontSize must be a positive number so invalid inputs are rejected early.
- Around line 1170-1182: The roxygen block for previewNetworkInBrowser is
missing a `@param` entry for nodeFontSize; add a line like "@param nodeFontSize
Numeric font size for node labels (default 12)." to the documentation block
above the previewNetworkInBrowser function so R CMD check won't warn and the
generated man/previewNetworkInBrowser.Rd includes this argument.
---
Duplicate comments:
In `@man/exportNetworkToHTML.Rd`:
- Line 12: Add the missing documentation for the nodeFontSize parameter in the
Rd file: ensure the symbol nodeFontSize = 12 appears in both the \usage and
\arguments sections of exportNetworkToHTML.Rd and add a matching descriptive
entry for nodeFontSize in the \arguments block (and mirror the same addition for
the other duplicate occurrence referenced), so the parameter is documented
consistently and R CMD check no longer flags an undocumented parameter.
---
Nitpick comments:
In `@R/getSubnetworkFromIndra.R`:
- Around line 35-37: The documentation for the filter_by_ptm_site parameter is
missing the exact expected PTM-site column names/format; update the Roxygen for
the getSubnetworkFromIndra function to list the required input columns (e.g.,
"gene", "site", "residue" or whatever the package expects) or add a link to the
input schema, and state the expected format (e.g., "S123" or "S123_phospho");
explicitly mention the column names used by the function (those parsed in
getSubnetworkFromIndra or its helper that checks PTM sites) so users know how to
structure their differential PTM abundance input before setting
filter_by_ptm_site = TRUE.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
R/getSubnetworkFromIndra.RR/utils_getSubnetworkFromIndra.RR/visualizeNetworksWithHTML.Rman/exportNetworkToHTML.Rd
🚧 Files skipped from review as they are similar to previous changes (1)
- R/utils_getSubnetworkFromIndra.R
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #68 +/- ##
==========================================
+ Coverage 57.89% 58.76% +0.87%
==========================================
Files 7 7
Lines 1356 1414 +58
==========================================
+ Hits 785 831 +46
- Misses 571 583 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Motivation and Context
This PR adds PTM-site-based filtering to INDRA-derived subnetworks to make extracted interactions more relevant for phospho/PTM-centric proteomics analyses. It refactors the INDRA filtering pipeline into discrete helper steps (PTM-site filtering then curation filtering) and exposes new visualization font-size parameters for HTML/Cytoscape outputs. The aim is to (1) allow optional removal of INDRA edges that do not match input PTM-site information and (2) separate curation adjustments into a dedicated helper.
Solution summary
Detailed changes
R/getSubnetworkFromIndra.R
R/utils_getSubnetworkFromIndra.R
R/visualizeNetworksWithHTML.R, man/exportNetworkToHTML.Rd, man/generateCytoscapeConfig.Rd, man/previewNetworkInBrowser.Rd
Documentation
Public API changes
Unit tests added or modified
Coding guidelines and potential issues