Skip to content

refactor: move optional arguments of layout functions behind the ellipsis#2762

Open
krlmlr wants to merge 3 commits into
mainfrom
claude/ellipsis-layout
Open

refactor: move optional arguments of layout functions behind the ellipsis#2762
krlmlr wants to merge 3 commits into
mainfrom
claude/ellipsis-layout

Conversation

@krlmlr

@krlmlr krlmlr commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Part of the repo-wide ellipsis migration coordinated in #2757
see that PR for the full rationale (CONTRIBUTING.md, Argument Order and the Ellipsis).
Based directly on main (the registry split landed via #2779); a single topic commit.

What this does

Inserts ... between the defining arguments (head) and the optional
modifiers
(tail) of 15 exported layout functions.
Arguments after ... become keyword-only.

  • Legacy positional or abbreviated calls are recovered by the generated
    ARG_HANDLE block and emit a single lifecycle::deprecate_soft("3.0.0", …)
    behavior is unchanged.
  • No defaults change, no arguments are renamed, deprecated functions untouched.
  • Registry: tools/migrations/layout.R; blocks regenerated via
    Rscript tools/generate-migrations.R (idempotent, CI-checked).
  • Rd usage/arguments updated mechanically (see note below).

New signatures

function head (positional) keyword-only tail
layout_as_bipartite() graph, types hgap, vgap, maxiter
layout_as_star() graph center, order
layout_as_tree() graph root, circular, rootlevel, mode, flip.y
layout_on_grid() graph width, height, dim
layout_randomly() graph dim
layout_with_dh() graph coords, maxiter, fineiter, cool.fact, weight.node.dist, weight.border, weight.edge.lengths, weight.edge.crossings, weight.node.edge.dist
layout_with_drl() graph use.seed, seed, options, weights, dim
layout_with_fr() graph coords, dim, niter, start.temp, grid, weights, minx, maxx, miny, maxy, minz, maxz, coolexp, maxdelta, area, repulserad, maxiter
layout_with_gem() graph coords, maxiter, temp.max, temp.min, temp.init
layout_with_graphopt() graph start, niter, charge, mass, spring.length, spring.constant, max.sa.movement
layout_with_kk() graph coords, dim, maxiter, epsilon, kkconst, weights, minx, maxx, miny, maxy, minz, maxz, niter, sigma, initemp, coolexp, start
layout_with_lgl() graph maxiter, maxdelta, area, coolexp, repulserad, cellsize, root
layout_with_sugiyama() graph layers, hgap, vgap, maxiter, weights, attributes
merge_coords() graphs, layouts method
norm_coords() layout xmin, xmax, ymin, ymax, zmin, zmax

Placement rethink: component_wise() and normalize() were originally
migrated here with an empty head (all arguments keyword-only) — such
signatures are now disallowed (the head must never be empty; see the updated
rule in #2757), so both were dropped from this PR and keep their original
signatures.

Notes for review

  • The environment used to prepare this PR cannot install igraph.r2cdocs
    (GitHub API unreachable), so man/*.Rd files were updated mechanically
    (usage + ... argument item) instead of via devtools::document().
    R CMD check's usage↔formals validation passes; a follow-up
    devtools::document() run may reflow whitespace but should produce no
    semantic diff.
  • Package tests that called these functions positionally were updated to
    named arguments (they are the same soft-deprecation user code will see).

@krlmlr
krlmlr force-pushed the claude/ellipsis-layout branch 2 times, most recently from e4aa32a to abdaf1b Compare July 26, 2026 11:09
@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if abdaf1b is merged into main:

  • ✔️as_adjacency_matrix: 776ms -> 779ms [-0.89%, +1.75%]
  • ✔️as_biadjacency_matrix: 748ms -> 748ms [-0.5%, +0.6%]
  • ✔️as_data_frame_both: 1.5ms -> 1.48ms [-2.76%, +1.01%]
  • ✔️as_long_data_frame: 3.92ms -> 3.92ms [-1.4%, +1.5%]
  • ✔️es_attr_filter: 2.7ms -> 2.71ms [-1.24%, +1.97%]
  • 🚀graph_from_adjacency_matrix: 121ms -> 120ms [-2.24%, -0.15%]
  • ✔️graph_from_data_frame: 3.35ms -> 3.38ms [-0.91%, +2.75%]
  • ✔️vs_attr_filter: 1.53ms -> 1.53ms [-2.61%, +2.6%]
  • ✔️vs_by_name: 973µs -> 985µs [-1.11%, +3.46%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@krlmlr
krlmlr marked this pull request as ready for review July 26, 2026 15:03
@krlmlr
krlmlr force-pushed the claude/ellipsis-layout branch 2 times, most recently from bcf8343 to 2669762 Compare July 26, 2026 15:20
@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2669762 is merged into main:

  • ✔️as_adjacency_matrix: 684ms -> 685ms [-1.51%, +1.61%]
  • ✔️as_biadjacency_matrix: 646ms -> 647ms [-1.29%, +1.57%]
  • ✔️as_data_frame_both: 1.28ms -> 1.27ms [-3.32%, +2.35%]
  • ✔️as_long_data_frame: 3.18ms -> 3.17ms [-2.33%, +1.91%]
  • ✔️es_attr_filter: 2.22ms -> 2.26ms [-1.31%, +4.86%]
  • ✔️graph_from_adjacency_matrix: 132ms -> 132ms [-1.36%, +0.91%]
  • ✔️graph_from_data_frame: 2.85ms -> 2.84ms [-2.66%, +1.95%]
  • ✔️vs_attr_filter: 1.27ms -> 1.28ms [-3.07%, +3.59%]
  • ✔️vs_by_name: 862µs -> 880µs [-2.2%, +6.3%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@krlmlr
krlmlr force-pushed the claude/ellipsis-layout branch from 2669762 to 63b7ade Compare July 26, 2026 16:23
…psis

Insert `...` between the defining arguments and the optional
modifiers of 17 functions,
following the zoning rules in CONTRIBUTING.md.
Legacy positional and abbreviated calls are recovered
by the generated ARG_HANDLE blocks
(registry: tools/migrations/layout.R)
and emit a single soft deprecation for igraph 3.0.0.
No defaults change and no arguments are renamed.

Functions: component_wise, layout_as_bipartite, layout_as_star, layout_as_tree, layout_on_grid, layout_randomly, layout_with_dh, layout_with_drl, layout_with_fr, layout_with_gem, layout_with_graphopt, layout_with_kk, layout_with_lgl, layout_with_sugiyama, merge_coords, norm_coords, normalize

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016M32izVHZPfxAqemAe4BrX
@krlmlr
krlmlr force-pushed the claude/ellipsis-layout branch from 63b7ade to 6fedb71 Compare July 26, 2026 19:25
@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8eef478 is merged into main:

  • ✔️as_adjacency_matrix: 945ms -> 941ms [-1.51%, +0.85%]
  • ✔️as_biadjacency_matrix: 876ms -> 883ms [-0.6%, +2.06%]
  • ✔️as_data_frame_both: 1.8ms -> 1.79ms [-2.61%, +1.2%]
  • ✔️as_long_data_frame: 4.48ms -> 4.47ms [-2.21%, +1.57%]
  • ✔️es_attr_filter: 3.07ms -> 3.08ms [-3.34%, +4.22%]
  • ✔️graph_from_adjacency_matrix: 166ms -> 168ms [-2.4%, +4.8%]
  • ✔️graph_from_data_frame: 3.8ms -> 3.76ms [-2.8%, +0.68%]
  • ✔️vs_attr_filter: 1.78ms -> 1.8ms [-2.24%, +4.38%]
  • ✔️vs_by_name: 1.19ms -> 1.17ms [-3.6%, +1.28%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

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.

2 participants