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)

@gvegayon

igraph 2.2.0 Update C core to version 0.10.17. See <https://github.com/igraph/rigraph/blob/20552ef94aed6ae4b23465ae8c7e4d3b0e558c71/src/vendor/cigraph/CHANGELOG.md> for a complete changelog, in particular the section "Breaking changes". - Generate almost all R implementations (#2047). - Expose `align_layout()` and add to `layout_nicely()` to align layout with axis automatically (#1907, #1957, #1958). - Expose `simple_cycles()` which lists all simple cycles (#1573, #1580). - Expose `is_complete()`, `is_clique()` and `is_ivs()` (#1316, #1388, #1581). - Expose `find_cycle()` (#1471, #1571). - Expose `feedback_vertex_set()` to find a minimum feedback vertex set in a graph (#1446, #1447, #1560). - Add `weights` parameter to `local_scan()` (#1082, #1448, #1982). - Add more layouts to `tkplot()` (#160, #1967). - Add `plot(mark.lwd = )` to change line width of mark.groups (#306, #1898). - Add `plot(vertex.label.angle = , vertex.label.adj = )` arguments to rotate vertex labels (#106, #1899). - Add relative size scaling to vertices in `plot()` (@gvegayon, #172). - Split `sample_bipartite()` into two functions for the G(n, m) and G(n, p) case (#630, #1692). - Implement multi attribute assignment (#55, #1916) and adding attributes via data frames (#1373, #1669, #1716). Support factors in `graph_from_data_frame()` (#34, #1829). - All `_hrg()` functions check their argument (#1074, #1699). - HRG printing with `type = "auto"` uses `"plain"` for large trees (#1879). - `get_edge_ids()` accepts data frames and matrices (#1663). - `igraph_version()` returns version of C core in an attribute (#1208, #1781). - Breaking change: change arguments default and order for `graph_from_lcf()` (#1858, #1872). - Breaking change: Subset assignment of a graph avoids addition of double edges and ignores loops unless the new `loops` argument is set to `TRUE` (#1662, #1661). - Breaking change: remove deprecated `neimode` parameter from `bfs()` and `dfs()` (#1105, #1526). - Breaking change: stricter deprecation of non-functional parameters of `layout_with_kk()` and `layout_with_fr()` (#1108, #1628). - `NA` attribute values are replaced with default values in `plot()` (#293, #1707). - `NA` checking only in from/to columns of edge data frame (#1906). - Keep vertex attribute type for `disjoint_union()` (#1640, #1909). - Error in bipartite projection if `type` is not a vertex attribute (#898, #1889). - Do not try to destroy non-initialized SIR objects upon error (#1888). - Added proper `NA` handling for matrix inputs (#917, #918, #1828). - Remove string matrix support from functions operating on biadjacency matrices (#1540, #1542, #1803). - Integer vectors are validated before transferring them to the C library (#1434, #1582). - Changed base location for `graph_from_graphdb()` and added tests (#1712, #1732). - Recycling of logical vectors when indexing into edge/vertex selectors now throws an error (#848, #1731). - Use `function()` instead of `(x)` in `arrow.mode` (#1722). - Temporarily disable generating an interface for `igraph_simple_cycles_callback()` as the framework for handling callback functions is not yet present. - Adjust loop position to vertex size in `plot()` (#1980). - Don't rescale plot coordinates to `[-1,1] x [-1,1]` by default (#1492, #1956, #1962). - Fail if `"layout"` attribute doesn't match the number of vertices (#1880). - Automatically arrange loops in `plot()` (#407, #556, #1881). - Vectorized drawing of arrows in `plot()` (#257, #1904). - Allow more than one edge label font family in `plot()` (#37, #1896). - Pie shapes now work as intended (#1882, #1883). - Loops not plotted on canvas (#1799, #1800). - Replace `NA` values in `label` attributes in `plot()` with default values (#1796, #1797). - Removed duplicated plotting of arrow heads (#640, #1709). - Correct mapping of edge label properties in plots when loops are present (#157, #1706). - Welcome Maëlle Salmon and David Schoch as authors (#1733), add author links (#1821). - Remove demos (#2008). - Add 2023 preprint (#1240, #1984). - Update allcontributors info (#1975). - Link to replacements of deprecated functions (#1823). - Add documentation of all file formats to `read_graph()` and `write_graph()` (#777, #1969). Recommend `saveRDS()` and `readRDS()` for saving and loading graphs (#1242, #1700). - Document return value of `make_clusters()` (#1794). - Clarify that `girth()` returns `Inf` for acyclic graphs (@eqmooring, #1831). - Clarify the use of weights in `layout_with_kk()`. - Refer to current latest version of R in troubleshooting page. - Fix typos in `laplacian_matrix()` documentation. - Document ellipsis in `cohesion()` (#971, #1985). - Correct the description of the `weights` parameter of `hits_scores()`. - Better describe output of `all_shortest_paths()` (#1029, #1778). - `make_graph()` now supports `"Groetzsch"` as an alias of `"Grotzsch"`. This change was implemented in the C core. - Update description of `order` parameter of `ego()` and related functions (#1746). - Added lifecycle table (#1525). - Add more about igraph.r2cdocs in the contributing guide (#1686, #1697). - Accelerate check if an index sequence corresponds to the entire list of vertices (#1427, #1818). - Faster single bracket querying of a graph (#1465, #1658).

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)

	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)))

Uh oh!

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

Uh oh!

Conversation

schochastics commented Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviator-app bot commented Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Aviator status

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

krlmlr Jan 19, 2025

Choose a reason for hiding this comment

Uh oh!

schochastics Jan 19, 2025

Choose a reason for hiding this comment

Uh oh!

schochastics Jan 19, 2025

Choose a reason for hiding this comment

Uh oh!

krlmlr Jan 19, 2025

Choose a reason for hiding this comment

Uh oh!

krlmlr Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

schochastics commented Jan 19, 2025

Uh oh!

Uh oh!

Uh oh!

krlmlr Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schochastics commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krlmlr commented Jan 20, 2025

Uh oh!

schochastics commented Jan 21, 2025

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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