feat: weight support for `eccentricity()` and `radius()`

[szhorvat]

Fixes #893

[aviator-app[bot]]

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.

[szhorvat]

Waiting on https://github.com/igraph/stimulus/issues/7

[szhorvat]

@maelle I need your help and advice here as this may require some lifecycle work.

This is a breaking change not just because of some changes to arguments, but also because the weight edge attribute is now used automatically, if present.

[maelle]

some changes to arguments, but also because the weight edge attribute is now used automatically, if present.

What changes to arguments exactly?

Regarding the use of the attribute, how did user use to opt in? Could there be a way for users to still opt in?

[szhorvat]

What changes to arguments exactly?

The weights parameter was added. It was not present previously.

mode now comes after ..., meaning that it must be used as a named argument, not as a positional one. radius(g, 'in') does not work anymore. One must use radius(g, mode='in').

Regarding the use of the attribute, how did user use to opt in? Could there be a way for users to still opt in?

It is a bad idea to make this opt-in as it creates inconsistencies and unpredictability in the long term. Let's not do that. If there is a weight attribute, and the function supports it, it should always be used automatically.

There's a way to opt out with weights=NA.

[szhorvat]

How shall we proceed with this?

[krlmlr]

I spoke too soon, this is actually a breaking change. Example for compatibility code:

diff --git a/R/community.R b/R/community.R
index dd1272ca36..7d9a0b83c4 100644
--- a/R/community.R
+++ b/R/community.R
@@ -677,7 +677,34 @@ modularity <- function(x, ...) {
 #' modularity(wtc)
 #' modularity(g, membership(wtc))
 #'
-modularity.igraph <- function(x, membership, weights = NULL, resolution = 1, directed = TRUE, ...) {
+modularity.igraph <- function(x, membership, ..., weights = NULL, resolution = 1, directed = TRUE) {
+
+  if (...length() > 0) {
+    lifecycle::deprecate_soft(
+      "3.0.0",
+      "modularity.igraph(... =)",
+      details = "The arguments `weights`, `resolution` and `directed` must be named."
+    )
+
+    dots <- list(...)
+    stopifnot(all(names2(dots) == ""))
+
+    if (is.null(weights) && length(dots) > 0) {
+      weights <- dots[[1]]
+      dots <- dots[-1]
+    }
+
+    if (missing(resolution) && length(dots) > 0) {
+      resolution <- dots[[1]]
+      dots <- dots[-1]
+    }
+
+    if (missing(directed) && length(dots) > 0) {
+      directed <- dots[[1]]
+      dots <- dots[-1]
+    }
+  }
+
   # Argument checks
   ensure_igraph(x)
   if (is.null(membership) || (!is.numeric(membership) && !is.factor(membership))) {

[szhorvat]

Can you please add tests for the new functions?

I won't be able to do this this week, but I'm happy to do it next week.

Could you please handle the rest, i.e. deciding on the function signature, where the ... should go, how much of a breaking change we're willing to make?

[maelle]

I'll come back to this PR next week.

[maelle]

• I added the lifecycle infrastructure. Does this seem correct @krlmlr?
• I added test cases. Do they make any sense @szhorvat? If so it would make sense to add the same test cases as examples especially for graph_center() that currently has no example.

[szhorvat]

• I added test cases. Do they make any sense @szhorvat? If so it would make sense to add the same test cases as examples especially for graph_center() that currently has no example.

I had a very quick look, seems fine as a smoke test, and yes, the test you added for graph_center() would actually make a very good example (perhaps with a smaller tree).

[maelle]

@szhorvat could you please fix the conflicts in src/cpp11.cpp, or tell me how to fix them? Thank you!

[krlmlr]

cpp11::cpp_register()

[krlmlr]

Needs the decor package.

[szhorvat]

I merged main into this branch and fixed the conflict by regenerating this file using R -q -e 'cpp11::cpp_register()'

Does this mess up Aviator because it won't be able to rebase anymore?

[krlmlr]

Aviator might be fine, I can't identify the reason why it sometimes fails to rebase.

[maelle]

thanks both!

[aviator-app[bot]]

This pull request failed to merge: PR cannot be automatically rebased, please rebase manually to continue. Remove the blocked label to re-queue.

Additional debug info: Failed to rebase this PR onto the latest changes from the base branch. You will probably need to rebase this PR manually and resolve conflicts).