Skip to content
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

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

Merged
merged 7 commits into from
Jan 30, 2025
Merged

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Jan 20, 2025

Also for add_edges() ?

The matrix case works already by accident, a two-row matrix will give the intended result. Should we soft-deprecate the matrix case to later support two-column matrices?

library(igraph)
set.seed(20250120)
g <- sample_gnp(20, 0.1)
g
#> IGRAPH dc0368b U--- 20 22 -- Erdos-Renyi (gnp) graph
#> + attr: name (g/c), type (g/c), loops (g/l), p (g/n)
#> + edges from dc0368b:
#>  [1]  1-- 3  1-- 5  3-- 8  7-- 8  5--11 10--11  2--12  3--14  5--14  1--15
#> [11] 10--15 11--15  4--16  6--16 12--16  1--17 15--17  3--18  4--18  8--19
#> [21] 11--19 12--19
x <- c(1, 3, 1, 5, 3, 8)
get_edge_ids(g, x)
#> [1] 1 2 3
m <- matrix(x, nrow = 2)
m
#>      [,1] [,2] [,3]
#> [1,]    1    1    3
#> [2,]    3    5    8
get_edge_ids(g, m)
#> [1] 1 2 3

Created on 2025-01-20 with reprex v2.1.1

Copy link
Contributor

aviator-app bot commented Jan 20, 2025

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 krlmlr enabled auto-merge (squash) January 20, 2025 09:21
@krlmlr krlmlr marked this pull request as draft January 20, 2025 09:21
auto-merge was automatically disabled January 20, 2025 09:21

Pull request was converted to draft

@schochastics
Copy link
Contributor

schochastics commented Jan 20, 2025

[.igraph is using the two-row matrix

res <- get_edge_ids(x, rbind(from, to), error = FALSE)

The case of a 2x2 matrix will be tough to properly handle because we dont know if the user suplied edges row or column wise

R/interface.R Outdated Show resolved Hide resolved
Copy link
Contributor Author

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

R/interface.R Outdated Show resolved Hide resolved
R/interface.R Outdated Show resolved Hide resolved
R/interface.R Outdated Show resolved Hide resolved
tests/testthat/test-interface.R Outdated Show resolved Hide resolved
@schochastics
Copy link
Contributor

schochastics commented Jan 23, 2025

The warnings in tests are all from "wrong" use of get_edge_ids() with 2xn matrices. These occur all for [.igraph and [<-.igraph. So we need to merge #1658 and #1661 first and rebase this PR.

cc @maelle If you have time (no rush) can you check if my use of lifecycle (in code and tests) is ok? (never actively used it before)

hmm ok, I get warnings locally due to the deprecation of 2 x n matrices. Seems not to be checked here? cc @krlmlr

Copy link
Contributor Author

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, can you please tweak and auto-merge when good?

R/interface.R Outdated Show resolved Hide resolved
R/interface.R Outdated Show resolved Hide resolved
R/interface.R Show resolved Hide resolved
tests/testthat/test-interface.R Outdated Show resolved Hide resolved
@schochastics schochastics marked this pull request as ready for review January 30, 2025 19:42
@schochastics schochastics merged commit 453fa1d into main Jan 30, 2025
11 checks passed
@schochastics schochastics deleted the f-edge-ids-df branch January 30, 2025 20:14
schochastics added a commit to schochastics/rigraph that referenced this pull request Feb 8, 2025
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