Overwriting attributes converts the new value to the class of the old value

[arcresu]

When setting an attribute that already exists (using V()<- or set_vertex_attr() and presumably also with edge attributes), igraph (1.2.6) will convert the new value to match the class of any existing value. For example:

library(igraph)
#> 
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#> 
#>     decompose, spectrum
#> The following object is masked from 'package:base':
#> 
#>     union

# does some processing and stores the result in a new integer attribute 'kind'
set_kind <- function(gr) {
  # gr <- delete_vertex_attr(gr, "kind")  # workaround
  set_vertex_attr(gr, "kind", value = vcount(gr))
}

# (1) processing a clean graph works as expected
g <- make_full_graph(n=1)
g <- set_kind(g)
class(V(g)$kind)
#> [1] "numeric"

# (2) this graph happens to have an existing attribute 'kind' with a different class;
#     the result now has the wrong class
g <- make_full_graph(n=1) %>% set_vertex_attr("kind", value = "big")
g <- set_kind(g)
class(V(g)$kind)
#> [1] "character"

^{Created on 2021-06-10 by the reprex package (v2.0.0)}

Basically I would expect the parts marked (1) and (2) to do the same thing, but the result actually depends on whatever value the attribute had before. This makes V() <- inconsistent with the normal R semantics of the <- operator which allow you to replace a variable with a value of a different class just fine.

To make the processing code robust against arbitrary input graphs, it's necessary to explicitly clear every attribute before setting it, but this becomes cumbersome.

[ntamas]

FWIW it looks like character cannot be converted to integer simply by doing an assignment, but it works the other way round:

# (1) converting a character attribute to integer doesn't work
> g <- make_full_graph(n=1) %>% set_vertex_attr("kind", value = "big")
> class(V(g)$kind)
#> [1] "character"
> V(g)$kind <- vcount(g)
> class(V(g)$kind)
#> [1] "character"
> V(g)$kind
#> [1] "1"

# (2) but converting an integer to a character does
> g <- make_full_graph(n=1) %>% set_vertex_attr("kind", value = vcount(g))
> class(V(g)$kind)
#> [1] "integer"
> V(g)$kind <- c("big")
> class(V(g)$kind)
#> [1] "character"

[krlmlr]

I'm about to enter a deep deep rabbit hole here.

To fix this in an idiomatic way, we need to be able to distinguish between:

• updating the attributes for all vertices (for which we would allow changing the type), and
• updating the attributes for only a subset of the vertices (for which we would be stricter and only allow casts to more generic types, e.g., from integer to character).

To support this in a strict way, V(g) would need to return an object that indicates that all vertices are addressed. Currently, V(g) isn't much different from V(g)[1:3] :

suppressMessages(library(igraph))
g <- make_ring(10)
dput(V(g))
#> structure(1:10, class = "igraph.vs", env = <weak reference>, graph = "79e455be-99fd-414c-a447-fcd86cdbacee")
dput(V(g)[1:3])
#> structure(1:3, env = <weak reference>, graph = "79e455be-99fd-414c-a447-fcd86cdbacee", class = "igraph.vs")
dput(V(g)[2:5])
#> structure(2:5, env = <weak reference>, graph = "79e455be-99fd-414c-a447-fcd86cdbacee", class = "igraph.vs")

^{Created on 2022-12-14 with reprex v2.0.2}

Ideally, V(g) would return an object that allows us to make this distinction. This could be as simple as an extra attribute (for maximum compatibility).

Should we go all in here?

[ntamas]

I guess yes; V(g) could return a structure where is_all = TRUE and any subsetting of V(g) could set it to is_all = FALSE; would that work? I'm not sure about the performance implications but it does not seem too severe.

[krlmlr]

Preparatory refactorings are done now, one final step still to do. We want to decide first if we can afford stricter recycling rules, a revdepcheck seems necessary.

[krlmlr]

Might want to revert the most recent PR #608 too, #607 is gone already.

[krlmlr]

Do as.igraph.vs() and as.igraph.es() need to carry over the new is_all attribute?

library(igraph, warn.conflicts = FALSE)

g <- make_full_graph(n = 1)

dput(V(g))
#> structure(1L, class = "igraph.vs", is_all = TRUE, env = <weak reference>, graph = "eb0db5f3-100d-462b-b8e2-bea733367953")
dput(igraph:::as.igraph.vs(g, V(g)))
#> 1

^{Created on 2023-01-15 with reprex v2.0.2}

[krlmlr]

Is this intended?

library(igraph, warn.conflicts = FALSE)

g <- make_full_graph(n = 10)

V(g)$foo <- 1
V(g)$foo
#>  [1] 1 1 1 1 1 1 1 1 1 1

vertex.attributes(g, 2:4) <- list(foo = 2:4)

V(g)$foo
#>  [1] NA  2  3  4 NA NA NA NA NA NA

^{Created on 2023-01-15 with reprex v2.0.2}

[ntamas]

Do as.igraph.vs() and as.igraph.es() need to carry over the new is_all attribute?

Well, what do we use the attribute for? I tried grepping through the source code in the dev branch and I see no uses of the is_complete_iterator() function yet. So I'd say it depends on what you want to use it for. If it's only meant to detect the situation when we might be updating the type of an attribute via V(g)$foo <- ... or E(g)$foo <- ..., then I would say it's not necessary to carry it over.

As for the second example, I would say it's probably a bug. It definitely does not match my naive expectation.