Skip to content

refactor: create ensure_igraph()#730

Merged
krlmlr merged 18 commits intoigraph:mainfrom
maelle:ensure
May 16, 2023
Merged

refactor: create ensure_igraph()#730
krlmlr merged 18 commits intoigraph:mainfrom
maelle:ensure

Conversation

@maelle
Copy link
Copy Markdown
Contributor

@maelle maelle commented Mar 20, 2023

Fix #702

@maelle

This comment was marked as resolved.

Sign in to view
@maelle

This comment was marked as resolved.

Sign in to view
@ntamas

This comment was marked as resolved.

Sign in to view
@krlmlr

This comment was marked as resolved.

Sign in to view
@ntamas

This comment was marked as resolved.

Sign in to view
@krlmlr

This comment was marked as resolved.

Sign in to view
@maelle

This comment was marked as resolved.

Sign in to view
@ntamas

This comment was marked as resolved.

Sign in to view
@codecov

This comment was marked as outdated.

Sign in to view
@maelle

This comment was marked as resolved.

Sign in to view
@maelle

This comment was marked as resolved.

Sign in to view
@maelle

This comment was marked as resolved.

Sign in to view
@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Mar 30, 2023

at the moment, the only places where I saw NULL is ok according to the argument check, don't seem they'd actually work with NULL: https://github.com/search?q=repo%3Aigraph%2Frigraph+is.null%28graph%29+language%3AR&type=code&l=R

@ntamas
Copy link
Copy Markdown
Member

ntamas commented Mar 30, 2023

centr_degree_tmax() and all the other centr_... functions would work with NULL if the user specifies nodes instead. These functions require only the number of nodes in the graph so the user can either specify the full graph or only the number of nodes. myscg() would not work with NULL indeed. The remaining occurrences are unrelated.

IMHO we can disallow NULL for ensure_igraph() and we can have ensure_optional_igraph() for the centr_... cases.

@krlmlr krlmlr changed the title refactor: create ensure_igraph() refactor: create ensure_igraph() Mar 30, 2023
@krlmlr

This comment was marked as resolved.

Sign in to view
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Mar 30, 2023

Did my best to resolve conflicts, sorry if I messed up

@ntamas

This comment was marked as resolved.

Sign in to view
@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Apr 3, 2023

How about

rigraph/R/scg.R

Line 709 in 7a70f0e

## Argument checks
? Can one only supply a matrix?

maelle
maelle commented Apr 3, 2023
View reviewed changes
R/attributes.R Outdated
#' @export
graph.attributes <- function(graph) {
ensure_igraph(graph)
.Call(C_R_igraph_mybracket2_copy, graph, igraph_t_idx_attr, igraph_attr_idx_graph)
Copy link
Copy Markdown
Contributor Author

@maelle maelle Apr 3, 2023

Choose a reason for hiding this comment

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

is this ok @krlmlr? seems to come from the conflict resolution?

Copy link
Copy Markdown
Contributor Author

@maelle maelle Apr 3, 2023

Choose a reason for hiding this comment

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

I made a commit removing duplicate code

maelle
maelle commented Apr 3, 2023
View reviewed changes
R/attributes.R Outdated
graph_attr_names <- function(graph) {
ensure_igraph(graph)

res <- .Call(C_R_igraph_mybracket2_names, graph, igraph_t_idx_attr, igraph_attr_idx_graph)
Copy link
Copy Markdown
Contributor Author

@maelle maelle Apr 3, 2023

Choose a reason for hiding this comment

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

here too @krlmlr

@ntamas
Copy link
Copy Markdown
Member

ntamas commented Apr 3, 2023

How about

rigraph/R/scg.R

Line 709 in 7a70f0e

## Argument checks

? Can one only supply a matrix?

Nice catch. Indeed the underlying igraph_scg_adjacency function can work from a graph, a matrix or a sparse matrix, but it only needs exactly one of these.

Note that the spectral coarse graining functions (i.e. everything that's in this chapter were removed in igraph 0.10 due to concerns about maintainability (in other words, no one really knew from the current dev team how this piece of code works). They now live in a separate project. If we want to keep these after upgrading the C core to igraph 0.10, we need to link to both projects.

@maelle maelle mentioned this pull request Apr 3, 2023
Closed
maelle
maelle commented Apr 3, 2023
View reviewed changes
R/operators.R Outdated
if (!all(sapply(graphs, is_igraph))) {
stop("Not a graph object")
}
purrr::walk(graphs, ensure_igraph)
Copy link
Copy Markdown
Contributor Author

@maelle maelle Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

example with more than one graph.

@maelle maelle marked this pull request as ready for review April 3, 2023 13:44
@maelle maelle requested a review from krlmlr April 11, 2023 14:25
krlmlr
krlmlr reviewed Apr 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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. How easy is it to split these changes between autogenerated and manual parts? Asking for #768.

Are we okay with the cli import?

@ntamas
Copy link
Copy Markdown
Member

ntamas commented Apr 17, 2023

cli import is okay for me.

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Apr 20, 2023

How easy is it to split these changes between autogenerated and manual parts?

You tell me 😉 It's still not 100% clear to me how autogeneration happens but I did modify tools/stimulus/types-RR.yaml, hopefully correctly?

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented May 15, 2023

My rhetoric question is a disguised form of saying "this PR is too big"... Let's see if the tests pass and merge, it looks good otherwise.

@ntamas
Copy link
Copy Markdown
Member

ntamas commented May 15, 2023

LGTM.

@krlmlr krlmlr merged commit e5b3fee into igraph:main May 16, 2023
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented May 16, 2023

Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@krlmlr krlmlr krlmlr left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

add a function assert_igraph()?

3 participants

@maelle @ntamas @krlmlr