Skip to content

perf: speed up .apply_modifiers() in make.R#2600

Draft
Copilot wants to merge 5 commits intomainfrom
copilot/remove-todo-speed-up-make-r
Draft

perf: speed up .apply_modifiers() in make.R#2600
Copilot wants to merge 5 commits intomainfrom
copilot/remove-todo-speed-up-make-r

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 27, 2026

Removes the ## TODO: speed this up comments in .apply_modifiers() by implementing the actual speedups.

Changes

  • without_attr: Replace three O(N) loops (get names → delete one-by-one) with three direct bulk assignments to structure(list(), names = character(0)) — O(1) regardless of attribute count.
  • with_vertex_ / with_edge_: Replace per-attribute set_vertex_attr/set_edge_attr calls (each doing a full C-level get+modify+set cycle) with a single get → in-memory loop with scalar recycling and length validation → single set. Reduces from N get+set pairs to 1 get + 1 set. Error messages correctly report make_() as the call context via the call parameter.
  • with_graph_: Replace per-attribute loop with the existing modify_list() helper + graph.attributes<- for a single get+set cycle.
  • Add snapshot tests for wrong-length attribute value errors in with_vertex_() and with_edge_().
  • By submitting this pull request, I assign the copyright of my contribution to The igraph development team.

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Copilot AI changed the title [WIP] Remove TODO comment from make.R perf: speed up .apply_modifiers() in make.R Mar 27, 2026
Copilot AI requested a review from schochastics March 27, 2026 19:04
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Apr 9, 2026

@copilot: Resolve conflicts and run tests, fix as needed.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 9, 2026

@copilot: Resolve conflicts and run tests, fix as needed.

Merged main, resolved conflicts, and fixed the CI failure (commit 7adb10e). The check was reporting NOTEs for setNames (from stats) and modifyList (from utils) not being imported. Replaced them with structure(list(), names = character(0)) and the existing modify_list() helper respectively.

Copilot AI requested a review from krlmlr April 9, 2026 15:21
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Apr 9, 2026

@copilot: Ensure this can be merged cleanly into main. The previous attempt still produces a branch with conflicts.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 9, 2026

@copilot: Ensure this can be merged cleanly into main. The previous attempt still produces a branch with conflicts.

Merged main cleanly in d35b162. The conflicts were in R/make.R and the snapshot file — resolved by combining our performance improvements with main's call parameter propagation (so errors now correctly report make_() instead of .apply_modifiers()).

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

can we remove the "## TODO: speed this up" in make.R?

3 participants