Skip to content

fix: as_adjacency_matrix(attr = NULL) is unweighted again - #2842

Merged
krlmlr merged 2 commits into
mainfrom
claude/fix-adjacency-attr-null-8mj8p1
Aug 16, 2026
Merged

fix: as_adjacency_matrix(attr = NULL) is unweighted again#2842
krlmlr merged 2 commits into
mainfrom
claude/fix-adjacency-attr-null-8mj8p1

Conversation

@krlmlr

@krlmlr krlmlr commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

attr and weights spell "unweighted" differently, and the deprecation shim forwards one to the other unchanged.

  • attr = NULL was the documented way to ask for a plain 0/1 matrix. CRAN 2.3.3's ?as_adjacency_matrix says: "attr: Either NULL or a character string ... If NULL a traditional adjacency matrix is returned."
  • weights = NULL means the opposite: use the weight edge attribute if the graph has one.

resolve_edge_weights() did weights <- attr, so every call that spelled out attr = NULL started returning a weighted matrix — silently, with no warning beyond the generic "use weights instead". A graph having a weight attribute is exactly the case where a caller bothers to ask for an unweighted matrix, so the inversion hits precisely the calls that meant it.

lifecycle::is_present(NULL) is TRUE — only the deprecated() sentinel counts as absent — so the branch really does see an explicit attr = NULL and has to translate it.

What changed

  • attr = NULL now maps to weights = NA (the new vocabulary for "explicitly unweighted"). A character attribute name is still forwarded unchanged.
  • The deprecation warning gains a detail line for the NULL case: `attr = NULL` becomes `weights = NA`, not `weights = NULL`. Following the bare advice literally would reintroduce the same inversion in user code.
  • @param attr documents the translation instead of claiming the value is always "a character edge attribute name".
  • Tests for both callers of resolve_edge_weights()as_adjacency_matrix() and as_biadjacency_matrix() — plus one for the warning text.

Reproducer

g <- make_graph(c(1, 2, 2, 3), directed = FALSE)
E(g)$weight <- c(0.5, 0.7)
as_adjacency_matrix(g, attr = NULL, sparse = FALSE)

CRAN 2.3.3 and this branch return a 0/1 matrix; main returns the 0.5/0.7 one.

Where this came from

Found while triaging the reverse-dependency failures in #2646. bnstruct calls as_adjacency_matrix(ig.mst, type = "both", attr = NULL) in buildJunctionTree-methods.R, on a minimum spanning tree that carries weights — the whole point of the call is to get the topology without them. Introduced by a6ace15 (#2677).

What this does not fix

The attr = NULL spelling is unambiguously a bug: the shim inverts an explicit request. The default is a separate, deliberate-looking question that this PR leaves alone.

as_adjacency_matrix(g) with no weight argument at all also changed meaning — weights = NULL picks up weight, where attr's default NULL did not. That is the item #2646 raises for sfclust, and it is also what breaks MetaNet: within_module_deg_z_score() and part_coeff() call as_adj(g, sparse = FALSE) in their weighted = FALSE branch and as_adj(g, sparse = FALSE, attr = "weight") in the other, so the unweighted branch now returns the weighted matrix and the Zi/Pi values land outside every role band.

Whether the new default is the intended 3.0.0 behaviour is a maintainer call — it is the same shape as the modularity() item in #2646, and it needs a NEWS entry either way. Flagging it here rather than changing it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01D1xpHRV7yVfgtJg4vp9P7z

claude added 2 commits August 16, 2026 06:33
`attr` and `weights` spell "unweighted" differently.
`attr = NULL` was the documented way to ask for a plain 0/1 matrix --
"If `NULL` a traditional adjacency matrix is returned" --
while `weights = NULL` means the opposite:
pick up the `weight` edge attribute if the graph has one.

The deprecation shim forwarded `attr` to `weights` unchanged,
so every call that spelled out `attr = NULL` started returning
a weighted matrix, silently and without a warning.
The graph having a `weight` attribute is exactly the case
where a caller bothers to ask for an unweighted matrix,
so the inversion hits precisely the calls that meant it.

Translate `NULL` to `weights = NA` instead,
and say so in the deprecation warning:
following the bare "use `weights` instead" advice literally
would reintroduce the same inversion in user code.

Both callers of `resolve_edge_weights()` are affected,
`as_adjacency_matrix()` and `as_biadjacency_matrix()`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D1xpHRV7yVfgtJg4vp9P7z
The detail line about `weights = NA` only fires for `attr = NULL`,
so snapshotting both spellings pins the difference
rather than asserting a substring of one of them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D1xpHRV7yVfgtJg4vp9P7z
@github-actions

Copy link
Copy Markdown
Contributor

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

  • ✔️as_adjacency_matrix: 807ms -> 797ms [-3.48%, +0.93%]
  • ✔️as_biadjacency_matrix: 802ms -> 803ms [-2.42%, +2.69%]
  • ✔️as_data_frame_both: 1.74ms -> 1.74ms [-2.08%, +2.15%]
  • ✔️as_long_data_frame: 4.09ms -> 4.16ms [-3.64%, +7.39%]
  • ✔️es_attr_filter: 2.76ms -> 2.76ms [-1.73%, +1.63%]
  • ✔️graph_from_adjacency_matrix: 146ms -> 145ms [-1.44%, +1.12%]
  • ✔️graph_from_data_frame: 3.76ms -> 3.92ms [-3.48%, +11.6%]
  • ✔️vs_attr_filter: 1.57ms -> 1.56ms [-2.16%, +1.19%]
  • ✔️vs_by_name: 1.03ms -> 1.1ms [-3.16%, +16.72%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@krlmlr
krlmlr merged commit 4662a55 into main Aug 16, 2026
9 checks passed
@krlmlr
krlmlr deleted the claude/fix-adjacency-attr-null-8mj8p1 branch August 16, 2026 09:13
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