Skip to content

Fix coloring of methods and add throughput lines in network vs runtime plots - #201

Open
oruebel wants to merge 4 commits into
mainfrom
fix_method_colors
Open

oruebel wants to merge 4 commits into
mainfrom
fix_method_colors

Conversation

@oruebel

@oruebel oruebel commented Oct 5, 2026

Copy link
Copy Markdown
Contributor
  • The colors for different methods were chosen within each function, which led to inconsistent colors in particular when select methods were for some reason missing in a plot. This PR adds a fixed color palette to ensure consistent assignment of colors to methods across all plots.
  • Add throughput (MB/s) lines to the plots showing the network traffic vs runtime

Copilot AI balanced review requested due to automatic review settings October 5, 2026 04:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Fallback colors remain subset-dependent, and throughput guides misrepresent non-byte metrics.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
What changed in this PR

Standardizes method colors across benchmark plots and adds throughput references to network-traffic-versus-runtime visualizations.

Changes:

  • Adds a shared method palette across plot types.
  • Adds dashed throughput guides, captions, and adjusted titles.
File Description
src/​nwb_benchmarks/​database/​_visualization.py Centralizes method colors and adds throughput overlays.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/nwb_benchmarks/database/_visualization.py Outdated
Comment thread src/nwb_benchmarks/database/_visualization.py Outdated
@oruebel
oruebel requested review from CodyCBakerPhD and rly October 5, 2026 04:39
@CodyCBakerPhD

Copy link
Copy Markdown
Collaborator

One minor recommendation, either/or:

  • Swap #9467BD for a color that also differs in brightness, for example a dark brown like #8C510A
  • Tell zarr s3 and zarr s3 force no consolidated apart by marker or line style instead, since they're variants of the same method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants