-
-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: g + vertices(1, 2, foo = 3)
works again, regression introduced in igraph 2.0.0
#1247
Conversation
Current Aviator status
This PR was merged using Aviator.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
R/operators.R
Outdated
names(e2)[wn] <- "name" | ||
unnamed_elements_indices <- which(!rlang::have_name(e2)) | ||
if (length(unnamed_elements_indices) == 1) { | ||
e2 <- rlang::set_names(e2[unnamed_elements_indices], "name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the readability of this line is not obviously better but since I was using rlang above...
R/operators.R
Outdated
} else if (is.null(names(e2))) { | ||
## No names at all, everything is a vertex name | ||
e2 <- list(name = unlist(e2, recursive = FALSE)) | ||
} else if (length(wn) == 0) { | ||
} else if (length(unnamed_elements_indices) == 0) { | ||
## If there are no non-named arguments, we are fine | ||
} else { | ||
## Otherwise, all unnamed arguments are collected and used as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the case that was broken as far as I understand. Not sure why it works to fix it here.
} | ||
la <- unique(sapply(e2, length)) | ||
res <- add_vertices(e1, la, attr = e2) | ||
# When adding vertices via +, all unnamed arguments are interpreted as vertex names of the new vertices. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comes from the docs
library("igraph")
#>
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#>
#> decompose, spectrum
#> The following object is masked from 'package:base':
#>
#> union
g <- make_empty_graph(1)
g
#> IGRAPH bd455eb D--- 1 0 --
#> + edges from bd455eb:
V(g)
#> + 1/1 vertex, from bd455eb:
#> [1] 1
g <- g +
vertices("a", "b", foo = 5)
g
#> IGRAPH 34a9a54 DN-- 3 0 --
#> + attr: name (v/c), foo (v/n)
#> + edges from 34a9a54 (vertex names):
V(g)
#> + 3/3 vertices, named, from 34a9a54:
#> [1] <NA> a b
V(g)$foo
#> [1] NA 5 5 Created on 2024-02-12 with reprex v2.1.0 |
} | ||
la <- unique(sapply(e2, length)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this supposed to be nv? I think my change here is the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Since we obviously didn't have any tests, can you please add some?
tests/testthat/test-operators.R
Outdated
expect_s3_class(V(g_all_unnamed), "igraph.vs") | ||
expect_identical(V(g_all_unnamed)$name, c( NA, "a", "b")) | ||
|
||
g_mix_named_unnamed <- g + vertices("a", "b", foo = 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens with foo = 6:7
and foo = 8:10
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test. The error also happens with v1.6.0 and seems logical.
I added a test but I'm surprised I need to use library("igraph")
#>
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#>
#> decompose, spectrum
#> The following object is masked from 'package:base':
#>
#> union
g <- make_empty_graph(1)
g
#> IGRAPH ea2b4e5 D--- 1 0 --
#> + edges from ea2b4e5:
V(g)
#> + 1/1 vertex, from ea2b4e5:
#> [1] 1
g <- g +
vertices("a", "b", foo = 5)
g
#> IGRAPH 6aaaab9 DN-- 3 0 --
#> + attr: name (v/c), foo (v/n)
#> + edges from 6aaaab9 (vertex names):
V(g)
#> + 3/3 vertices, named, from 6aaaab9:
#> [1] <NA> a b
V(g)$name
#> <NA> <NA> <NA>
#> NA "a" "b"
V(g)$foo
#> [1] NA 5 5 Created on 2024-02-13 with reprex v2.1.0 And with v1.6.0, it's the same actually library("igraph")
#>
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#>
#> decompose, spectrum
#> The following object is masked from 'package:base':
#>
#> union
g <- make_empty_graph(1)
g
#> IGRAPH c331734 D--- 1 0 --
#> + edges from c331734:
V(g)
#> + 1/1 vertex, from c331734:
#> [1] 1
g <- g +
vertices("a", "b", foo = 5)
g
#> IGRAPH dc14fb9 DN-- 3 0 --
#> + attr: name (v/c), foo (v/n)
#> + edges from dc14fb9 (vertex names):
V(g)
#> + 3/3 vertices, named, from dc14fb9:
#> [1] <NA> a b
V(g)$name
#> <NA> <NA> <NA>
#> NA "a" "b"
V(g)$foo
#> [1] NA 5 5 Created on 2024-02-13 with reprex v2.1.0 We can discuss later today. 😸 |
vertices(1, 2, foo = 3)
works again, regression introduced in igraph 2.0.0
b42f5d3
to
2ce6614
Compare
vertices(1, 2, foo = 3)
works again, regression introduced in igraph 2.0.0g + vertices(1, 2, foo = 3)
works again, regression introduced in igraph 2.0.0
Fix #1231
Partly refactoring for understanding for now.