Skip to content

refactor: Rename algo to algorithm in page_rank(), feedback_arc_set() and feedback_vertex_set() - #2859

Open
krlmlr wants to merge 2 commits into
mainfrom
claude/issue-2788-subagents-faw1t1
Open

refactor: Rename algo to algorithm in page_rank(), feedback_arc_set() and feedback_vertex_set()#2859
krlmlr wants to merge 2 commits into
mainfrom
claude/issue-2788-subagents-faw1t1

Conversation

@krlmlr

@krlmlr krlmlr commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Part of #2788 (PR 1 of a stack of 6, least controversial first); implements the algoalgorithm leg of #526.

What this does

  • Renames the algo argument to algorithm in page_rank(), feedback_arc_set() and feedback_vertex_set() — the three functions the Comprehensive plan: argument renames for 3.0.0 #2788 survey lists under the algo spelling. All three already carry their 3.0.0 keyword-only migration entries with algo after ..., so the rename is a pure registry change (algo = algorithm bare-symbol rename in tools/migrations/), and legacy algo = callers are recovered with the same single soft-deprecation as positional callers:

    Calling `page_rank()` with positional or abbreviated arguments was deprecated in igraph 3.0.0.
    i Detected call: page_rank(graph, algo)
    i Use instead: page_rank(graph, algorithm = )
    
  • Abbreviations that could mean either spelling (a, al, alg) are rejected as ambiguous by the regenerated ARG_HANDLE guards.

  • The deprecated page.rank() wrapper keeps its frozen algo formal (now documented locally with a deprecated badge, since @inheritParams page_rank no longer carries it) and forwards to algorithm.

  • Tests: named uses updated, plus new coverage that the legacy name is recovered (snapshot for page_rank(algo = ), expect_deprecated for the two feedback functions).

Out of scope

method (12 algorithm-choice + 2 comparison-metric functions), implementation (2) and impl (1) are the open "algorithm family" questions in #2788 and are not touched here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RTPj4qNv2FWui6etixZeuR


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

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

  • ✔️as_adjacency_matrix: 761ms -> 762ms [-0.87%, +1.09%]
  • ✔️as_biadjacency_matrix: 769ms -> 768ms [-0.88%, +0.6%]
  • ✔️as_data_frame_both: 1.89ms -> 1.88ms [-1.95%, +1.01%]
  • ✔️as_long_data_frame: 4.75ms -> 4.77ms [-1.01%, +1.74%]
  • ✔️es_attr_filter: 3.31ms -> 3.33ms [-0.66%, +1.89%]
  • ✔️graph_from_adjacency_matrix: 120ms -> 121ms [-0.68%, +0.98%]
  • ✔️graph_from_data_frame: 4.08ms -> 4.06ms [-1.88%, +0.79%]
  • ❗🐌vs_attr_filter: 1.76ms -> 1.79ms [+0.2%, +2.85%]
  • ✔️vs_by_name: 1.14ms -> 1.16ms [-2.05%, +4.46%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

…c_set()` and `feedback_vertex_set()` (#2788, #526)

The legacy `algo` spelling is recovered by the generated ARG_HANDLE blocks
and soft-deprecated;
abbreviations of both spellings are guarded as ambiguous.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RTPj4qNv2FWui6etixZeuR
`ambiguous_tags()` charmatched a candidate prefix against `match_names`,
so any rename whose old name is a prefix of the new one (`algo` ->
`algorithm`) flagged `a`, `al` and `alg` as ambiguous even though both
candidates resolve to the same argument. Those calls used to work with a
soft deprecation and hard-errored instead, with a message that was
factually wrong.

Dedupe on `match_to` so a prefix is ambiguous only when the names it
matches resolve to different arguments of the new API. Regenerating
narrows the `page_rank()` guard to `d` (`damping` vs `directed`, a real
ambiguity), drops the guards from `feedback_arc_set()` and
`feedback_vertex_set()` entirely, and also drops the same latent false
positive from `migration_fixture()` (`weight` -> `weights`).
`migration_fixture_shadow()` keeps rejecting `at =`, where the two
candidates really do have distinct targets.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@schochastics

Copy link
Copy Markdown
Contributor

Reviewed the rename itself and it looks correct: no drift in the generated blocks, no stale algo = callers left apart from the *_impl() C wrappers (which rightly keep it), and page.rank() forwards algorithm = algo by exact name so it doesn't double-warn.

I did find one regression in the generated ambiguity guard, fixed in 47d1fa6.

The bug

ambiguous_tags() in tools/generate-migrations.R charmatch()ed a candidate prefix against match_names — the union of renamed-away old names and new tail names. For a rename whose old name is a prefix of the new one (algoalgorithm), alg matches both entries, so charmatch() returned 0 and the tag was emitted into the guard. But both candidates resolve to the same argument, so nothing was actually ambiguous:

page_rank(g, alg = "arpack")       # soft deprecation before, hard error after
feedback_arc_set(g, alg = "exact_ip")  # newly guarded by this PR

The error message was also factually wrong ("matches multiple arguments of page_rank()"), and it contradicted tools/migrations/README.md. This isn't specific to these three functions — every rename in the #2788 stack where the old name prefixes the new one would hit it.

The fix

A prefix is ambiguous only when the names it matches resolve to different targets, so the predicate now dedupes on match_to:

hits <- startsWith(names, p)
length(unique(entry$match_to[hits])) > 1L

Regenerating narrows or drops four guards, and keeps the ones that matter:

  • page_rank(): c("a", "al", "alg", "d")c("d"). d is genuinely ambiguous (damping vs directed) and still errors; alg = recovers under the deprecation.
  • feedback_arc_set() / feedback_vertex_set(): guard gone — it only ever held the false algo/algorithm prefixes.
  • migration_fixture(): guard gone too. It was flagging wweigh for exactly the same reason (weightweights), so this was a pre-existing false positive in the fixture, unrelated to the rename.
  • migration_fixture_shadow(): still rejects at =, the one case where the two candidates really do have distinct targets (weights vs the retained deprecated attr).

Test changes

  • weig = moves from the error-message snapshots to the deprecation snapshots in test-migration-fixture.R.
  • New generator-level test for ambiguous_tags(): a prefix-rename yields no tags, two distinct targets still yield "d".
  • test-centrality.R gains page_rank(star, alg = "prpack") (recovers) and page_rank(star, d = 0.5) (still errors), so the distinction is pinned at the function level.
  • Comments in generate-migrations.R and tools/migrations/fixture.R updated to state the rule correctly.

Full suite green locally: 9311 pass, 0 fail.

I kept this in the same PR rather than splitting it: the bug only becomes user-visible through this rename, and the fix changes these three functions' generated blocks, so a separate PR would leave this one mergeable-but-broken and need a rebase on the generated code anyway.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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

  • ✔️as_adjacency_matrix: 806ms -> 804ms [-1.45%, +0.87%]
  • ✔️as_biadjacency_matrix: 801ms -> 794ms [-1.82%, +0.09%]
  • ✔️as_data_frame_both: 1.73ms -> 1.75ms [-0.79%, +2.75%]
  • ✔️as_long_data_frame: 4.18ms -> 4.19ms [-1.68%, +2.04%]
  • ✔️es_attr_filter: 2.85ms -> 2.8ms [-3.43%, +0.13%]
  • ✔️graph_from_adjacency_matrix: 143ms -> 143ms [-0.5%, +1.44%]
  • ✔️graph_from_data_frame: 3.9ms -> 3.9ms [-1.28%, +1.4%]
  • ✔️vs_attr_filter: 1.59ms -> 1.58ms [-1.7%, +1.37%]
  • ✔️vs_by_name: 1.06ms -> 1.07ms [-1.92%, +4.26%]
    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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants