Skip to content

Refactor PTM visualization #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tonywu1999 merged 4 commits into from
Feb 25, 2026
Merged

Refactor PTM visualization #69

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2f6eac
first attempt for PTM site as a node
tonywu1999 Feb 25, 2026
b4b84d6
change PTM site color
tonywu1999 Feb 25, 2026
4ff6f5d
place child PTM nodes adjacent to parent nodes and add compound nodes
tonywu1999 Feb 25, 2026
c5d4a50
coderabbit comment
tonywu1999 Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
194 changes: 183 additions & 11 deletions R/visualizeNetworksWithHTML.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,30 +296,122 @@ getEdgeStyle <- function(interaction, category, edge_type) {
}

createNodeElements <- function(nodes, displayLabelType = "id") {
# Map logFC to colors if logFC column exists
if ("logFC" %in% names(nodes)) {
node_colors <- mapLogFCToColor(nodes$logFC)
} else {
node_colors <- rep("#D3D3D3", nrow(nodes)) # Default color
node_colors <- rep("#D3D3D3", nrow(nodes))
}

# Determine which column to use for labels
label_column <- if(displayLabelType == "hgncName" && "hgncName" %in% names(nodes)) {
label_column <- if (displayLabelType == "hgncName" && "hgncName" %in% names(nodes)) {
"hgncName"
} else {
"id"
}

apply(cbind(nodes, color = node_colors), 1, function(row) {
# Use the appropriate label, fallback to id if hgncName is missing/empty
display_label <- if(label_column == "hgncName" && !is.na(row['hgncName']) && row['hgncName'] != "") {
row['hgncName']
node_elements <- c()
ptm_elements <- c()
emitted_proteins <- c()
emitted_compounds <- c()
emitted_ptm_nodes <- c()
emitted_ptm_edges <- c()

Comment thread
tonywu1999 marked this conversation as resolved.
# Pre-compute which protein ids have at least one PTM site row,
# so we know upfront whether a compound wrapper is needed
has_ptm_sites <- if ("Site" %in% names(nodes)) {
ids_with_sites <- unique(nodes$id[!is.na(nodes$Site) & trimws(nodes$Site) != ""])
ids_with_sites
} else {
c()
}

for (i in seq_len(nrow(nodes))) {
row <- nodes[i, ]
color <- node_colors[i]
has_site <- "Site" %in% names(nodes) && !is.na(row$Site) && trimws(row$Site) != ""

display_label <- if (label_column == "hgncName" && !is.na(row$hgncName) && row$hgncName != "") {
row$hgncName
} else {
row['id']
row$id
}

paste0("{ data: { id: '", row['id'], "', label: '", display_label, "', color: '", row['color'], "' } }")
})
needs_compound <- row$id %in% has_ptm_sites
compound_id <- paste0(row$id, "__compound__")

# Emit invisible compound container node once per protein that has PTM children
if (needs_compound && !(compound_id %in% emitted_compounds)) {
node_elements <- c(node_elements,
paste0("{ data: { id: '", escape_js_string(compound_id),
"', node_type: 'compound' } }")
)
emitted_compounds <- c(emitted_compounds, compound_id)
}

# Emit protein node once, assigning it to the compound if one exists
if (!(row$id %in% emitted_proteins)) {
parent_field <- if (needs_compound) {
paste0(", parent: '", escape_js_string(compound_id), "'")
} else {
""
}
node_elements <- c(node_elements,
paste0("{ data: { id: '", escape_js_string(row$id),
"', label: '", escape_js_string(display_label),
"', color: '", color,
"', node_type: 'protein'",
parent_field,
" } }")
)
emitted_proteins <- c(emitted_proteins, row$id)
}

# Emit one PTM child node + attachment edge per individual site
if (has_site) {
sites <- trimws(unlist(strsplit(as.character(row$Site), "[_,;|]")))
sites <- unique(sites[sites != ""])

for (site in sites) {
ptm_node_id <- paste0(row$id, "__ptm__", site)
safe_ptm_id <- escape_js_string(ptm_node_id)
safe_parent <- escape_js_string(row$id)
safe_site <- escape_js_string(site)

# PTM node also belongs to the same compound container
if (!(ptm_node_id %in% emitted_ptm_nodes)) {
ptm_elements <- c(ptm_elements,
paste0("{ data: { id: '", safe_ptm_id,
"', label: '", safe_site,
"', color: '", color,
"', parent_protein: '", safe_parent,
"', parent: '", escape_js_string(compound_id), "'",
", node_type: 'ptm' } }")
)
emitted_ptm_nodes <- c(emitted_ptm_nodes, ptm_node_id)
}

ptm_edge_id_raw <- paste0(row$id, "__ptm_edge__", site)
if (!(ptm_edge_id_raw %in% emitted_ptm_edges)) {
ptm_edge_id <- escape_js_string(ptm_edge_id_raw)
ptm_elements <- c(ptm_elements,
paste0("{ data: { id: '", ptm_edge_id,
"', source: '", safe_parent,
"', target: '", safe_ptm_id,
"', edge_type: 'ptm_attachment',",
" category: 'ptm_attachment',",
" interaction: '',",
" color: '", color, "',",
" line_style: 'dotted',",
" arrow_shape: 'none',",
" width: 1.5,",
" tooltip: '' } }")
)
emitted_ptm_edges <- c(emitted_ptm_edges, ptm_edge_id_raw)
}
}
}
}

return(c(node_elements, ptm_elements))
}
Comment on lines +311 to 415
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Please add regression tests for the new PTM emission/layout paths.

This PR adds non-trivial branching here (compound wrapping, per-site dedup, and post-layout PTM positioning), and this file still has uncovered changed lines in the patch report. Add targeted tests for repeated sites (within/across rows), multi-site rows, and multi-PTM sibling placement.

I can draft testthat cases for these scenarios if you want.

Also applies to: 693-728

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@R/visualizeNetworksWithHTML.R` around lines 311 - 415, The review asks for
regression tests covering the new PTM emission and layout branches in
visualizeNetworksWithHTML.R: add testthat cases that exercise has_ptm_sites /
needs_compound logic and deduplication by emitted_compounds, emitted_ptm_nodes,
and emitted_ptm_edges; specifically create tests for (1) repeated identical
sites within one row, (2) identical sites across multiple rows for the same
protein id, (3) rows with multiple distinct sites, and (4) multiple PTM siblings
to verify compound parent assignment and unique PTM node/edge ids; call the
function (the wrapper that returns node_elements/ptm_elements) with crafted
nodes data frames and assert the returned vector contains the expected compound
node id (paste0(id,'__compound__')), unique ptm node ids
(paste0(id,'__ptm__',site)), and single attachment edges per site, and add these
tests to testthat suite so lines covered in has_ptm_sites, the for-loop PTM
emission, and dedupe branches are exercised.


createEdgeElements <- function(edges, nodes = NULL) {
Expand Down Expand Up @@ -493,6 +585,49 @@ generateCytoscapeConfig <- function(nodes, edges,
`source-arrow-shape` = "triangle",
`target-arrow-shape` = "triangle"
)
),
list(
selector = "node[node_type = 'ptm']",
style = list(
shape = "ellipse",
width = "20px",
height = "20px",
`background-color` = "data(color)", # <-- was hardcoded "#9932CC"
`border-color` = "#333", # <-- neutral border instead of purple
`border-width` = 1.5,
label = "data(label)",
`font-size` = "8px",
`font-weight` = "normal",
color = "#000000", # <-- dark text works across the logFC palette
`text-valign` = "center",
`text-halign` = "center",
`text-wrap` = "wrap",
`text-max-width` = "18px"
)
),
# --- PTM attachment edge style (hide label, keep it subtle) ---
list(
selector = "edge[edge_type = 'ptm_attachment']",
style = list(
`line-style` = "dotted",
`line-color` = "#9932CC",
width = 1.5,
`target-arrow-shape` = "none",
`source-arrow-shape` = "none",
label = "", # no label on these connector edges
`z-index` = 0 # render behind main edges
)
),
list(
selector = "node[node_type = 'compound']",
style = list(
`background-opacity` = 0,
`border-width` = 0,
`border-opacity` = 0,
`padding` = "10px",
label = "",
`z-index` = 0
)
)
)

Expand Down Expand Up @@ -555,6 +690,43 @@ generateJavaScriptCode <- function(config) {
layout: ", layout_js, "
});

// After layout completes, reposition PTM nodes directly beside their parent protein
cy.on('layoutstop', function() {
var ptmNodes = cy.nodes('[node_type = \"ptm\"]');
ptmNodes.forEach(function(ptmNode) {
var parentId = ptmNode.data('parent_protein');
var parentNode = cy.getElementById(parentId);
if (parentNode.length === 0) return;

var parentPos = parentNode.position();
var parentW = parentNode.outerWidth();
var parentH = parentNode.outerHeight();
var ptmR = ptmNode.outerWidth() / 2; // PTM node is a small circle

// Collect all PTM siblings so we can fan them around the parent
var siblings = cy.nodes('[parent_protein = \"' + parentId + '\"]');
var idx = siblings.indexOf(ptmNode);
var total = siblings.length;
Comment on lines +707 to +709
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Avoid interpolating raw parentId into the selector string.

If an ID contains selector-significant characters, sibling lookup can fail or mis-select. Prefer filtering by data value instead of string-building selectors.

Proposed fix 
-            var siblings = cy.nodes('[parent_protein = \"' + parentId + '\"]');
+            var siblings = ptmNodes.filter(function(n) {
+                return n.data('parent_protein') === parentId;
+            });
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@R/visualizeNetworksWithHTML.R` around lines 707 - 709, The selector
construction using string interpolation of parentId is unsafe; instead select
candidate nodes and filter by their data value to avoid
selector-special-character issues: replace the cy.nodes('[parent_protein = "' +
parentId + '"]') usage with a safer approach that first grabs nodes (e.g.,
cy.nodes() or cy.nodes('[parent_protein]')) and then .filter(...) comparing
node.data('parent_protein') === parentId to produce siblings, keeping the
subsequent idx = siblings.indexOf(ptmNode) and total = siblings.length logic
unchanged.


// Distribute siblings evenly across the bottom arc of the parent
// angleStart/End in radians: spread across bottom 180 degrees
var angleStart = Math.PI * 0.15;
var angleEnd = Math.PI * 0.85;
var angle = total === 1
? Math.PI / 2 // single PTM: directly below center
: angleStart + (angleEnd - angleStart) * (idx / (total - 1));

// Place PTM node just outside the parent border
var offsetX = (parentW / 2 + ptmR + 4) * Math.cos(angle);
var offsetY = (parentH / 2 + ptmR + 4) * Math.sin(angle);

ptmNode.position({
x: parentPos.x + offsetX,
y: parentPos.y + offsetY
});
});
});

// Create tooltip element
var tooltip = document.createElement('div');
tooltip.style.cssText = `
Expand Down
Loading