Pie chart visualization for mapper graphs - #105
ishikaghosh2201 wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The updated dist_fit bounding logic can convert the integer search bound to a float (breaking the binary search), and a few plotting API/formatting issues should be corrected for correctness and consistency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds pie-chart node glyph rendering for mapper graphs, enabling per-node visualization of categorical label composition, and introduces optional node sizing proportional to point counts (via node_points recorded during computeMapper).
Changes:
- Add
node_pointstracking tocomputeMapperoutput graphs to support downstream per-node label aggregation. - Add
node_label_counts, pie-glyph rendering utilities, andpie_plot, plus aMapperGraph.draw_pie()convenience method. - Add tests covering label counting, error conditions, smoke rendering, and
size_by_pointszoom scaling behavior; bump package version.
File summaries
| File | Description |
|---|---|
cereeberus/cereeberus/compute/computemapper.py |
Records per-node original point indices (node_points) on mapper graph output. |
cereeberus/cereeberus/draw/draw.py |
Adds pie plotting utilities (node_label_counts, _pie_image, pie_plot) and refactors edge drawing into _draw_edges. |
cereeberus/cereeberus/reeb/mapper.py |
Adds MapperGraph.draw_pie() wrapper for pie-chart visualization. |
tests/test_pie_plot.py |
New tests for pie plotting, label counting, and size scaling behavior. |
tests/test_computemapper.py |
Adds regression test ensuring computeMapper populates node_points. |
cereeberus/cereeberus/distance/interleave.py |
Adjusts dist_fit binary search bounding logic (note: unrelated to pie plotting). |
pyproject.toml |
Version bump to 0.1.17 and minor formatting cleanup. |
Review details
Suppressed comments (1)
cereeberus/cereeberus/draw/draw.py:316
- The
pie_plotsignature isn’t formatted like the rest of this module (no spaces after commas, very long line). Reformatting (or runningblack) improves readability and avoids formatter churn in future diffs.
def pie_plot(R,labels,categories=None,colors=None,zoom=0.15,size_by_points=False,min_zoom=0.08,max_zoom=0.3,with_edges=True,with_legend=True,with_labels=True,cpx=0.1,cpy=0.1,ax=None):
- Files reviewed: 7/8 changed files
- Comments generated: 7
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| except ValueError: # infeasible assignment | ||
| low = mid + 1 | ||
|
|
||
| high = min(high, best_bound - 1) # to tighten the upper bound on the search space. this tries to go higher |
2df8a35 to
d600c37
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues affect edge rendering, sizing, autoscaling, transparency, and empty-graph handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
cereeberus/cereeberus/draw/draw.py:415
AnnotationBboxartists and networkx text labels do not contribute their positions toAxes.relim(). Therefore, whenwith_edges=Falseor the graph has no edges, autoscaling uses no node data and pies can be clipped or entirely outside the default limits; update the data limits fromR.pos_fbefore callingautoscale_view().
ax.relim()
ax.autoscale_view()
cereeberus/cereeberus/draw/draw.py:296
fig.canvas.buffer_rgba()captures the opaque figure and axes backgrounds here, so everyOffsetImageis a white square rather than a transparent circular pie. Those squares obscure edges, labels, and neighboring glyphs; make the figure/axes patches transparent (and hide the axes) before rendering the image.
fig = plt.figure(figsize=(px / dpi, px / dpi), dpi=dpi)
pie_ax = fig.add_axes([0, 0, 1, 1])
pie_ax.set_aspect("equal")
cereeberus/cereeberus/reeb/mapper.py:143
- The method defaults to
cpx=cpy=1.0, whilepie_plotandReebGraph.drawuse0.1; consequentlyMapperGraph.draw_pie()draws multi-edges with a much larger, inconsistent curvature than the helper it delegates to. Align these defaults withpie_plotunless the larger curvature is intentional and documented.
def draw_pie(self, labels, categories=None, colors=None, zoom=0.15, size_by_points=False, min_zoom=0.08, max_zoom=0.3, with_edges=True, with_legend=True, with_labels=True, cpx=1.0, cpy=1.0, ax=None):
- Files reviewed: 6/7 changed files
- Comments generated: 3
- Review effort level: Lite
Co-authored-by: ishikaghosh2201 <112980412+ishikaghosh2201@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues affect pie sizing, visibility, transparency, multiedge handling, and wrapper defaults.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
cereeberus/cereeberus/draw/draw.py:413
AnnotationBboxobjects are added withax.add_artist, so they do not contribute their data coordinates to the axes' data limits. Consequentlyrelim()/autoscale_view()only sees edge lines (or no data at all whenwith_edges=False), and disconnected nodes or edge-less graphs can place the pie glyphs outside the visible plot. Include the node positions in the data limits before autoscaling.
ax.relim()
ax.autoscale_view()
cereeberus/cereeberus/draw/draw.py:296
buffer_rgba()captures the figure and axes patches, which are opaque white by default, so everyOffsetImageis a square white tile rather than a transparent pie glyph. This is visible on non-white axes and when glyphs overlap; make both patches transparent before rendering.
fig = plt.figure(figsize=(px / dpi, px / dpi), dpi=dpi)
pie_ax = fig.add_axes([0, 0, 1, 1])
pie_ax.set_aspect("equal")
cereeberus/cereeberus/draw/draw.py:37
- Using
keys=Truefixes the tuple length, butline_loop_indexstill assumes key0precedes key1and that key1proves a parallel edge. NetworkX keys are identifiers whose values and iteration order can change; for example, removing key 0 and re-adding it can yield[1, 0], makingline_index.remove(...)raiseValueError. Classify duplicate(u, v)pairs instead of relying on numeric key values.
edge_list = list(R.edges(keys=True))
cereeberus/cereeberus/reeb/mapper.py:143
- The wrapper forwards
cpxandcpyas1.0by default, whilepie_plotand the existingReebGraph.drawuse0.1. A defaultdraw_pie()therefore changes the established edge curvature for multiedges instead of using the delegated plotting function's defaults; keep these wrapper defaults aligned.
def draw_pie(self, labels, categories=None, colors=None, zoom=0.15, size_by_points=False, min_zoom=0.08, max_zoom=0.3, with_edges=True, with_legend=True, with_labels=True, cpx=1.0, cpy=1.0, ax=None):
- Files reviewed: 6/7 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate issues in drawing and edge handling remain unresolved.
Review details
Suppressed comments (2)
cereeberus/cereeberus/draw/draw.py:419
AnnotationBboxand text artists do not contribute their positions toAxes.relim(), so these calls only autoscale to edge line data. Isolated mapper nodes (or any call withwith_edges=False) can therefore fall outside the limits and have their pies clipped or invisible. Add everyR.pos_fcoordinate to the axis data limits before callingautoscale_view.
ax.relim()
ax.autoscale_view()
cereeberus/cereeberus/draw/draw.py:37
- Although
keys=Truefixes the tuple-length failure,line_loop_indexstill assumes that parallel edges are keyed exactly 0 and 1, with key 0 encountered first. A validMultiDiGraphwith only key 1 (for example after removing key 0) reaches the key-1 branch and.index(..., 0)raisesValueError; determine parallelism from endpoint groups/counts instead of key values.
edge_list = list(R.edges(keys=True))
- Files reviewed: 6/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Description
Adds pie-chart visualizations to mapper graph nodes via a new
draw_piemethod, showing the categorical breakdown of original data points assigned to each node. Also adds optional point-count-based sizing (size_by_points), so nodes with more points render larger pie glyphs than nodes with fewer.Motivation and Context
Mapper graph nodes represent clusters of original data points, but there was previously no way to visualise categorical metadata (independent of whatever built the graph) at the node level. This closes #90.
How has this been tested?
tests/test_pie_plot.pycovering:node_label_countscorrectness on a small known point-to-node mappingnode_label_countsraisingValueErroron manually-built graphs (nonode_pointsattribute)pie_plot/draw_piesmoke testssize_by_points=Truecorrectly scales zoom in proportion to each node's point count (verified via node-to-zoom correlation, not just min/max)size_by_points=Falsepreserves the original fixed-zoom behaviourZeroDivisionErrorcompute_mapper.ipynbagainst circles-dataset examples with both structured (angular quadrant) and random multi-category labelsTypes of changes
Checklist
pyproject.tomlfile if a new version needs to be pushed to pypi. Note that if the number isn't incremented, the package will not be pushed to pypi, which is useful if this PR is only for updating documentation.make formatto clean up the code withblack.make tests).