Skip to content

feat: Add attribute combination support to graph operators#2676

Merged
krlmlr merged 25 commits into
mainfrom
feat-attrib_comb
Jul 9, 2026
Merged

feat: Add attribute combination support to graph operators#2676
krlmlr merged 25 commits into
mainfrom
feat-attrib_comb

Conversation

@schochastics

Copy link
Copy Markdown
Contributor

fix #57

@schochastics

Copy link
Copy Markdown
Contributor Author

devtools::document() doesnt work locally for me

Comment thread tests/testthat/test-operators.R Outdated
Comment thread R/attributes.R
Comment thread R/operators.R
Comment thread R/operators.R Outdated
Comment thread R/operators.R
@github-actions

Copy link
Copy Markdown
Contributor

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

  • ✔️as_adjacency_matrix: 777ms -> 779ms [-0.27%, +0.78%]
  • ✔️as_biadjacency_matrix: 777ms -> 779ms [-0.47%, +1.17%]
  • ✔️as_data_frame_both: 1.61ms -> 1.59ms [-2.88%, +1.5%]
  • ✔️as_long_data_frame: 4ms -> 4ms [-1.29%, +1.34%]
  • ✔️es_attr_filter: 2.8ms -> 2.82ms [-1.01%, +2.48%]
  • ✔️graph_from_adjacency_matrix: 147ms -> 148ms [-0.73%, +1.43%]
  • ✔️graph_from_data_frame: 3.56ms -> 3.56ms [-1.83%, +1.32%]
  • ✔️vs_attr_filter: 1.65ms -> 1.63ms [-3.3%, +0.25%]
  • ✔️vs_by_name: 1.06ms -> 1.06ms [-2.31%, +2.11%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@igraph igraph deleted a comment from github-actions Bot Jun 10, 2026
schochastics and others added 3 commits June 10, 2026 12:27
…lper

Address review feedback on #2676:
- Rename the new graph.attr.comb/vertex.attr.comb/edge.attr.comb arguments of
  union()/intersection()/compose()/disjoint_union() to snake_case (these args
  are new in this PR, so no deprecation is needed).
- Extract rename_attr_if_needed() back out of combine.attrs() for the "rename"
  strategy, preserving the historical overwrite-on-clash semantics under chains
  of %du%.
- Add a clarifying comment on the backward-compatible "rename" default.
- Move the make_named_pair() test helper into helper.R.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tted names

Follow-up to review feedback on #2676 ("clean up edge.attr.comb"):

- simplify(), as_undirected() and contract() gain snake_case
  edge_attr_comb / vertex_attr_comb arguments via the argument-migration
  infrastructure (tools/migrations.R). The old dotted names still work and
  emit a single lifecycle::deprecate_soft() warning.
- Rename the matching igraph option keys edge.attr.comb -> edge_attr_comb and
  vertex.attr.comb -> vertex_attr_comb, with back-compatible aliasing in
  igraph_opt()/igraph_options(); the dotted keys still read and set, soft-
  deprecated. Update the stimulus codegen default (types-RR.yaml) and the
  regenerated *_impl defaults to match.

Deprecated wrappers (as.undirected(), contract.vertices()) are left frozen;
their snapshot now records the resulting layered deprecation warnings.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

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

  • ✔️as_adjacency_matrix: 731ms -> 728ms [-1.46%, +0.89%]
  • ✔️as_biadjacency_matrix: 735ms -> 740ms [-0.52%, +1.76%]
  • ✔️as_data_frame_both: 1.51ms -> 1.5ms [-2.15%, +1.45%]
  • ✔️as_long_data_frame: 4.06ms -> 4.05ms [-3.39%, +2.93%]
  • ✔️es_attr_filter: 2.9ms -> 2.93ms [-3.59%, +5.69%]
  • ✔️graph_from_adjacency_matrix: 118ms -> 119ms [-0.35%, +2.21%]
  • ✔️graph_from_data_frame: 3.54ms -> 3.58ms [-1.51%, +4.05%]
  • ✔️vs_attr_filter: 1.66ms -> 1.66ms [-2.81%, +3.13%]
  • ✔️vs_by_name: 1.05ms -> 1.09ms [-4.13%, +11.57%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Comment thread R/par.R Outdated
schochastics and others added 3 commits June 16, 2026 20:59
…e combination in operators

The `graph_attr_comb` parameter in `union()`, `intersection()`, `disjoint_union()`, and `compose()` now defaults to `igraph_opt("graph_attr_comb")` (which is `"rename"` by default) instead of a hard-coded `"rename"`. This allows users to globally configure graph attribute combination behavior via `igraph_options()`.
@github-actions

Copy link
Copy Markdown
Contributor

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

  • 🚀as_adjacency_matrix: 753ms -> 714ms [-5.86%, -4.62%]
  • 🚀as_biadjacency_matrix: 760ms -> 719ms [-6.08%, -4.74%]
  • ✔️as_data_frame_both: 1.53ms -> 1.55ms [-1.38%, +3.28%]
  • ✔️as_long_data_frame: 3.91ms -> 3.9ms [-1.4%, +1.38%]
  • ✔️es_attr_filter: 2.72ms -> 2.76ms [-1.17%, +4.01%]
  • 🚀graph_from_adjacency_matrix: 119ms -> 112ms [-6.8%, -4.45%]
  • ❗🐌graph_from_data_frame: 3.41ms -> 3.46ms [+0.56%, +2.73%]
  • ✔️vs_attr_filter: 1.59ms -> 1.61ms [-4.11%, +6.69%]
  • ✔️vs_by_name: 1.02ms -> 1.02ms [-2.37%, +1.64%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@schochastics

Copy link
Copy Markdown
Contributor Author

@maelle it is now *_combine ;-)

@github-actions

Copy link
Copy Markdown
Contributor

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

  • ✔️as_adjacency_matrix: 680ms -> 679ms [-0.8%, +0.74%]
  • ❗🐌as_biadjacency_matrix: 678ms -> 683ms [+0.03%, +1.19%]
  • ✔️as_data_frame_both: 1.49ms -> 1.48ms [-2.85%, +1.13%]
  • ✔️as_long_data_frame: 3.94ms -> 3.92ms [-2.14%, +1.01%]
  • ✔️es_attr_filter: 2.7ms -> 2.72ms [-1.14%, +2.69%]
  • ✔️graph_from_adjacency_matrix: 110ms -> 110ms [-1.23%, +1.6%]
  • ✔️graph_from_data_frame: 3.32ms -> 3.4ms [-1.65%, +6.38%]
  • ✔️vs_attr_filter: 1.5ms -> 1.5ms [-2.3%, +2.33%]
  • ✔️vs_by_name: 942µs -> 946µs [-1.47%, +2.21%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@maelle

maelle commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Yay 🎉

@maelle maelle 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.

Thank you!!

Comment thread R/attributes.R Outdated
Comment thread R/attributes.R Outdated
Comment thread R/attributes.R Outdated
if (is.na(x)) {
idx <- pmatch(tolower(x), known_names)
if (is.na(idx)) {
if (!allow_rename && identical(tolower(x), "rename")) {

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.

It'd be more logical to me to invert the two

if (identical(tolower(x), "rename") && !allow_rename) {

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.

why identical btw?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you seem to hold a grudge against identical() 😛
Just very defensive type-strict scalar comparison. Probably == would work here but its igraph, so better be save 😉

Comment thread R/attributes.R
if (is.na(idx)) {
if (!allow_rename && identical(tolower(x), "rename")) {
cli::cli_abort(
"{.val rename} is only supported by graph operators ({.fn union}, {.fn intersection}, {.fn compose}, {.fn disjoint_union}), not by this function."

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.

Rephrase to

  • Can't use rename with function X (possible to add its name)
  • Hint: rename is only supported by blablabla

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This gets surprisingly complicated so I would opt out of that and keep it as is

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.

Why not

Can't use rename with this function (not naming it)
Hint: rename is only supported by blablabla

Comment thread R/attributes.R Outdated
Comment thread R/attributes.R
Comment thread tests/testthat/_snaps/conversion.md Outdated
Comment thread tests/testthat/test-operators.R Outdated
Comment thread tests/testthat/test-operators.R Outdated
Comment thread tests/testthat/test-operators.R Outdated
@github-actions

Copy link
Copy Markdown
Contributor

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

  • ✔️as_adjacency_matrix: 798ms -> 797ms [-1.22%, +1.08%]
  • ✔️as_biadjacency_matrix: 781ms -> 778ms [-1.47%, +0.6%]
  • ✔️as_data_frame_both: 1.62ms -> 1.64ms [-2.16%, +4.51%]
  • ✔️as_long_data_frame: 4.25ms -> 4.27ms [-3.59%, +4.67%]
  • ✔️es_attr_filter: 2.88ms -> 2.93ms [-2.8%, +6.51%]
  • ❗🐌graph_from_adjacency_matrix: 126ms -> 128ms [+0.61%, +3.22%]
  • ✔️graph_from_data_frame: 3.55ms -> 3.62ms [-0.43%, +4.31%]
  • ✔️vs_attr_filter: 1.67ms -> 1.64ms [-4.2%, +0.92%]
  • ✔️vs_by_name: 1.13ms -> 1.14ms [-2.13%, +4.11%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@krlmlr krlmlr 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.

I'd like to review of the "par" code in a separate PR. Let's keep the original option names dotted in this PR (revert as a single commit).

Comment thread R/par.R Outdated

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.

Suggested change
if (...length() == 0) {

@github-actions

Copy link
Copy Markdown
Contributor

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

  • ✔️as_adjacency_matrix: 731ms -> 730ms [-0.69%, +0.44%]
  • ✔️as_biadjacency_matrix: 737ms -> 732ms [-1.99%, +0.56%]
  • ✔️as_data_frame_both: 1.46ms -> 1.48ms [-0.66%, +2.8%]
  • ✔️as_long_data_frame: 3.91ms -> 3.9ms [-1.35%, +1.06%]
  • ✔️es_attr_filter: 2.66ms -> 2.65ms [-1.57%, +0.47%]
  • ✔️graph_from_adjacency_matrix: 117ms -> 117ms [-2%, +1.62%]
  • ❗🐌graph_from_data_frame: 3.39ms -> 3.45ms [+0.12%, +3.49%]
  • 🚀vs_attr_filter: 1.55ms -> 1.5ms [-6.79%, -0.14%]
  • ✔️vs_by_name: 978µs -> 977µs [-2.9%, +2.62%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

…eparate PR

Per review feedback (PR #2676), the snake_case renaming of the igraph
options and of the *.attr.comb function arguments (with soft-deprecation of
the dotted names) is "par" code that will be reviewed in its own PR. The
full renaming work is preserved on branch `par-attr-comb-options`.

This reverts the renaming here, keeping the original dotted option names:

- R/par.R: restore dotted `vertex.attr.comb` / `edge.attr.comb` options and
  drop the alias/soft-deprecation machinery. Keep the feature's new
  `graph.attr.comb` option (dotted), defaulting to "rename".
- simplify() / as_undirected() / contract(): drop the argument-migration
  blocks; keep the dotted `edge.attr.comb` / `vertex.attr.comb` arguments
  (community.R, conversion.R, simple.R, aaa-*.R, migrations.R, types-RR.yaml,
  and the associated tests/snapshots revert to main).
- union() / intersection() / compose() / disjoint_union(): the new
  attribute-combination arguments use dotted names
  (graph.attr.comb / vertex.attr.comb / edge.attr.comb).

Also addresses review comment r3473306646: use `...length() == 0` instead of
`nargs() == 0` in igraph_i_options().

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schochastics added a commit that referenced this pull request Jul 8, 2026
… names

Split out from #2676 per review feedback: the renaming of the igraph
options and the *.attr.comb function arguments (with soft-deprecation of
the dotted names) is carried here so it can be reviewed on its own.

- igraph options vertex.attr.comb / edge.attr.comb (and the new
  graph.attr.comb) are renamed to snake_case *_combine, with
  back-compatible aliasing in igraph_opt() / igraph_options(); the dotted
  keys still read and set, soft-deprecated (R/par.R).
- simplify() / as_undirected() / contract() gain snake_case
  edge_attr_combine / vertex_attr_combine arguments via the
  argument-migration infrastructure (tools/migrations.R); dotted names
  keep working and soft-deprecate.
- union() / intersection() / compose() / disjoint_union() use the
  snake_case *_combine argument names.
- Updates stimulus codegen defaults (types-RR.yaml), regenerated *_impl
  defaults, and the affected tests/snapshots.

Retains the igraph_i_options() ...length() fix from the base branch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

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

  • 🚀as_adjacency_matrix: 700ms -> 679ms [-4.45%, -1.43%]
  • ✔️as_biadjacency_matrix: 689ms -> 684ms [-2.25%, +0.73%]
  • ✔️as_data_frame_both: 1.4ms -> 1.37ms [-4.25%, +0.69%]
  • ✔️as_long_data_frame: 3.61ms -> 3.63ms [-1.17%, +2.5%]
  • ✔️es_attr_filter: 2.67ms -> 2.72ms [-1.79%, +5.32%]
  • ✔️graph_from_adjacency_matrix: 134ms -> 135ms [-2.08%, +3.89%]
  • ✔️graph_from_data_frame: 3.07ms -> 3.04ms [-2.94%, +1.14%]
  • ✔️vs_attr_filter: 1.45ms -> 1.43ms [-4.21%, +1.19%]
  • ✔️vs_by_name: 928µs -> 926µs [-2.27%, +1.86%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@krlmlr
krlmlr merged commit decddf1 into main Jul 9, 2026
1 check passed
@krlmlr
krlmlr deleted the feat-attrib_comb branch July 9, 2026 08:42
schochastics added a commit that referenced this pull request Jul 15, 2026
… names

Split out from #2676 per review feedback: the renaming of the igraph
options and the *.attr.comb function arguments (with soft-deprecation of
the dotted names) is carried here so it can be reviewed on its own.

- igraph options vertex.attr.comb / edge.attr.comb (and the new
  graph.attr.comb) are renamed to snake_case *_combine, with
  back-compatible aliasing in igraph_opt() / igraph_options(); the dotted
  keys still read and set, soft-deprecated (R/par.R).
- simplify() / as_undirected() / contract() gain snake_case
  edge_attr_combine / vertex_attr_combine arguments via the
  argument-migration infrastructure (tools/migrations.R); dotted names
  keep working and soft-deprecate.
- union() / intersection() / compose() / disjoint_union() use the
  snake_case *_combine argument names.
- Updates stimulus codegen defaults (types-RR.yaml), regenerated *_impl
  defaults, and the affected tests/snapshots.

Retains the igraph_i_options() ...length() fix from the base branch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

R: API for combining attributes from different graphs

3 participants