fix!: Subset assignment of a graph avoids addition of double edges and ignores loops unless the new `loops` argument is set to `TRUE` #1661

schochastics · 2025-01-19T06:14:58Z

This PR refactors single bracket manipulating of a graph (g[1:3,4:6] <- 2) (#1465).

The only real change is the use of get_edge_ids instead of the old [.igraph to get edge ids which makes the function more readable, slightly faster and a lower memory footprint.

Fixes an unintended behaviour (fix #1662)

aviator-app · 2025-01-19T06:15:01Z

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.

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.

krlmlr

Thanks, I like the size and intention of this PR.

krlmlr · 2025-01-19T06:47:07Z

R/indexing.R

    } else {
-      todel <- unlist(x[[i, j, ..., edges = TRUE]])
+      edge_pairs <- expand.grid(i, j)
+      edge_ids <- get_edge_ids(x, c(rbind(edge_pairs[, 1], edge_pairs[, 2])))


Is this covered by tests?

Suggested change

edge_ids <- get_edge_ids(x, c(rbind(edge_pairs[, 1], edge_pairs[, 2])))

edge_ids <- get_edge_ids(x, as.vector(t(edge_pairs)))

The interface of get_edge_ids() is interesting. Should we extend that to accept two-column data frames?

Is this covered by tests?

Going through the existing tests, I realize there are some gaps. I will add a set of tests for this functionality

The interface of get_edge_ids() is interesting. Should we extend that to accept two-column data frames?

It has been bothering me mildly for years as a user that edges need to be supplied as a vector (same with add_edges() and delete_edges() and probably more). However that's required by the c core. It might be too much of a fundamental change at this point?

The c(rbind(...)) pattern is probably fine, I forgot the semantics for vectors:

c(rbind(1:3, 4:6)) #> [1] 1 4 2 5 3 6 c(t(data.frame(1:3, 4:6))) #> [1] 1 4 2 5 3 6 as.vector(t(data.frame(1:3, 4:6))) #> [1] 1 4 2 5 3 6

^{Created on 2025-01-19 with reprex v2.1.1}

There are two layers here: the C core and the R interface. We should provide an idiomatic R user interface that translates to what the C core needs.

c(rbind(...)) is faster for data frames by an order of magnitude, but not for matrices:

df <- as.data.frame(cbind(1:30, 4:33)) bench::mark( c(t(df)), c(rbind(df[, 1], df[, 2])), c(rbind(df[[1]], df[[2]])) ) #> # A tibble: 3 × 6 #> expression min median `itr/sec` mem_alloc `gc/sec` #> <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl> #> 1 c(t(df)) 12.96µs 14.51µs 57926. 74.4KB 34.8 #> 2 c(rbind(df[, 1], df[, 2])) 5.33µs 5.99µs 157537. 576B 47.3 #> 3 c(rbind(df[[1]], df[[2]])) 3.65µs 4.26µs 219083. 576B 43.8 m <- cbind(1:30, 4:33) bench::mark( c(t(m)), c(rbind(m[, 1], m[, 2])) ) #> # A tibble: 2 × 6 #> expression min median `itr/sec` mem_alloc `gc/sec` #> <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl> #> 1 c(t(m)) 820ns 983.94ns 964028. 576B 96.4 #> 2 c(rbind(m[, 1], m[, 2])) 984ns 1.15µs 791706. 576B 0

^{Created on 2025-01-20 with reprex v2.1.1}

Draft PR for new UI in #1663.

R/indexing.R

schochastics · 2025-01-19T15:43:26Z

should probably be blocked until #1662 is resolved

R/indexing.R

krlmlr · 2025-01-20T09:22:50Z

R/indexing.R

    } else {
-      todel <- unlist(x[[i, j, ..., edges = TRUE]])
+      edge_pairs <- expand.grid(i, j)
+      edge_ids <- get_edge_ids(x, c(rbind(edge_pairs[, 1], edge_pairs[, 2])))


c(rbind(...)) is faster for data frames by an order of magnitude, but not for matrices:

df <- as.data.frame(cbind(1:30, 4:33)) bench::mark( c(t(df)), c(rbind(df[, 1], df[, 2])), c(rbind(df[[1]], df[[2]])) ) #> # A tibble: 3 × 6 #> expression min median `itr/sec` mem_alloc `gc/sec` #> <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl> #> 1 c(t(df)) 12.96µs 14.51µs 57926. 74.4KB 34.8 #> 2 c(rbind(df[, 1], df[, 2])) 5.33µs 5.99µs 157537. 576B 47.3 #> 3 c(rbind(df[[1]], df[[2]])) 3.65µs 4.26µs 219083. 576B 43.8 m <- cbind(1:30, 4:33) bench::mark( c(t(m)), c(rbind(m[, 1], m[, 2])) ) #> # A tibble: 2 × 6 #> expression min median `itr/sec` mem_alloc `gc/sec` #> <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl> #> 1 c(t(m)) 820ns 983.94ns 964028. 576B 96.4 #> 2 c(rbind(m[, 1], m[, 2])) 984ns 1.15µs 791706. 576B 0

^{Created on 2025-01-20 with reprex v2.1.1}

Draft PR for new UI in #1663.

R/indexing.R

schochastics · 2025-01-20T13:31:09Z

~~waiting for #1663 to be merged (then rebase and adapt)~~

krlmlr · 2025-01-20T21:03:52Z

Do you want to add more tests here?

schochastics · 2025-01-21T09:18:28Z

Do you want to add more tests here?

Yes!

krlmlr

Looks good to me after #1663. Please auto-squash or squash when good on your end.

…d ignores loops unless the new `loops` argument is set to `TRUE` (igraph#1661)

schochastics added 2 commits January 19, 2025 07:10

cleanup graph manipulation Part 1

b7d284d

cleanup graph manipulation Part 2

d9154a5

schochastics mentioned this pull request Jan 19, 2025

perf: Faster single bracket querying of a graph #1658

Merged

krlmlr reviewed Jan 19, 2025

View reviewed changes

apply incident to all nodes of i or j

8b42e85

schochastics marked this pull request as draft January 19, 2025 11:40

schochastics mentioned this pull request Jan 19, 2025

Intended behaviour of [<-.igraph #1662

Closed

schochastics added 2 commits January 19, 2025 20:13

switched from sapply to incident_edges

7fcfea9

new manipulation rules implemented

f540d9f

krlmlr reviewed Jan 20, 2025

View reviewed changes

resolved review comments

2b0d7f4

krlmlr changed the title ~~refactor: single bracket manipulating of a graph (#1465)~~ fix!: Subset assignment of a graph avoids addition of double edges and ignores loops unless the new loops argument is set to TRUE Jan 20, 2025

added more tests and fixed old tests

5e04178

krlmlr approved these changes Jan 23, 2025

View reviewed changes

schochastics mentioned this pull request Jan 23, 2025

feat: get_edge_ids() accepts data frames and matrices #1663

Merged

schochastics marked this pull request as ready for review January 23, 2025 20:07

schochastics merged commit cee57e1 into igraph:main Jan 23, 2025
22 checks passed

schochastics added a commit to schochastics/rigraph that referenced this pull request Jan 27, 2025

fix!: Subset assignment of a graph avoids addition of double edges an…

b064a94

…d ignores loops unless the new `loops` argument is set to `TRUE` (igraph#1661)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: Subset assignment of a graph avoids addition of double edges and ignores loops unless the new `loops` argument is set to `TRUE` #1661

fix!: Subset assignment of a graph avoids addition of double edges and ignores loops unless the new `loops` argument is set to `TRUE` #1661

schochastics commented Jan 19, 2025 •

edited

Loading

aviator-app bot commented Jan 19, 2025 •

edited

Loading

krlmlr left a comment

krlmlr Jan 19, 2025

schochastics Jan 19, 2025

schochastics Jan 19, 2025

krlmlr Jan 19, 2025

krlmlr Jan 20, 2025

schochastics commented Jan 19, 2025

krlmlr Jan 20, 2025

schochastics commented Jan 20, 2025 •

edited

Loading

krlmlr commented Jan 20, 2025

schochastics commented Jan 21, 2025

krlmlr left a comment

	edge_ids <- get_edge_ids(x, c(rbind(edge_pairs[, 1], edge_pairs[, 2])))
	edge_ids <- get_edge_ids(x, as.vector(t(edge_pairs)))

fix!: Subset assignment of a graph avoids addition of double edges and ignores loops unless the new loops argument is set to TRUE #1661

fix!: Subset assignment of a graph avoids addition of double edges and ignores loops unless the new loops argument is set to TRUE #1661

Conversation

schochastics commented Jan 19, 2025 • edited Loading

aviator-app bot commented Jan 19, 2025 • edited Loading

Current Aviator status

krlmlr left a comment

Choose a reason for hiding this comment

krlmlr Jan 19, 2025

Choose a reason for hiding this comment

schochastics Jan 19, 2025

Choose a reason for hiding this comment

schochastics Jan 19, 2025

Choose a reason for hiding this comment

krlmlr Jan 19, 2025

Choose a reason for hiding this comment

krlmlr Jan 20, 2025

Choose a reason for hiding this comment

schochastics commented Jan 19, 2025

krlmlr Jan 20, 2025

Choose a reason for hiding this comment

schochastics commented Jan 20, 2025 • edited Loading

krlmlr commented Jan 20, 2025

schochastics commented Jan 21, 2025

krlmlr left a comment

Choose a reason for hiding this comment

fix!: Subset assignment of a graph avoids addition of double edges and ignores loops unless the new `loops` argument is set to `TRUE` #1661

fix!: Subset assignment of a graph avoids addition of double edges and ignores loops unless the new `loops` argument is set to `TRUE` #1661

schochastics commented Jan 19, 2025 •

edited

Loading

aviator-app bot commented Jan 19, 2025 •

edited

Loading

schochastics commented Jan 20, 2025 •

edited

Loading