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

chore: deprecate scale argument of centr_eigen() and centr_eigen_tmax() #1625

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 48 additions & 6 deletions R/centralization.R
Original file line number Diff line number Diff line change
Expand Up @@ -448,8 +448,8 @@ centr_clo_tmax <- centralization_closeness_tmax_impl
#' @param graph The input graph.
#' @param directed logical scalar, whether to use directed shortest paths for
#' calculating eigenvector centrality.
#' @param scale Whether to rescale the eigenvector centrality scores, such that
#' the maximum score is one.
#' @param scale `r lifecycle::badge("deprecated")` Ignored. Computing
#' eigenvector centralization requires normalized eigenvector centrality scores.
#' @param options This is passed to [eigen_centrality()], the options
#' for the ARPACK eigensolver.
#' @param normalized Logical scalar. Whether to normalize the graph level
Expand Down Expand Up @@ -481,7 +481,29 @@ centr_clo_tmax <- centralization_closeness_tmax_impl
#' centr_eigen(g0)$centralization
#' centr_eigen(g1)$centralization
#' @cdocs igraph_centralization_eigenvector_centrality
centr_eigen <- centralization_eigenvector_centrality_impl
centr_eigen <- function(graph,
directed = FALSE,
scale = deprecated(),
options = arpack_defaults(),
normalized = TRUE) {

if (lifecycle::is_present(scale)) {
lifecycle::deprecate_soft(
"2.1.3",
"centr_eigen(scale = )",
details = "The function always behaves as if `scale` were TRUE.
The argument will be removed in the future."
)
}

centralization_eigenvector_centrality_impl(
graph = graph,
directed = directed,
options = options,
normalized = normalized,
scale = TRUE
)
}

#' Theoretical maximum for eigenvector centralization
#'
Expand All @@ -493,8 +515,8 @@ centr_eigen <- centralization_eigenvector_centrality_impl
#' given.
#' @param directed logical scalar, whether to consider edge directions
#' during the calculation. Ignored in undirected graphs.
#' @param scale Whether to rescale the eigenvector centrality scores,
#' such that the maximum score is one.
#' @param scale `r lifecycle::badge("deprecated")` Ignored. Computing
#' eigenvector centralization requires normalized eigenvector centrality scores.
#' @return Real scalar, the theoretical maximum (unnormalized) graph
#' eigenvector centrality score for graphs with given vertex count and
#' other parameters.
Expand All @@ -510,4 +532,24 @@ centr_eigen <- centralization_eigenvector_centrality_impl
#' `/`(centr_eigen_tmax(g))
#' centr_eigen(g, normalized = TRUE)$centralization
#' @cdocs igraph_centralization_eigenvector_centrality_tmax
centr_eigen_tmax <- centralization_eigenvector_centrality_tmax_impl
centr_eigen_tmax <- function(graph = NULL,
nodes = 0,
directed = FALSE,
scale = deprecated()) {

if (lifecycle::is_present(scale)) {
lifecycle::deprecate_soft(
"2.1.3",
"centr_eigen_tmax(scale = )",
details = "The function always behaves as if `scale` were TRUE.
The argument will be removed in the future."
)
}

centralization_eigenvector_centrality_tmax_impl(
graph = graph,
nodes = nodes,
directed = directed,
scale = TRUE
)
}
6 changes: 3 additions & 3 deletions man/centr_eigen.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 8 additions & 3 deletions man/centr_eigen_tmax.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions man/centralization.evcent.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions man/centralization.evcent.tmax.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions tests/testthat/_snaps/centralization.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# centr_eigen_tmax() deprecated argument

Code
c <- centr_eigen_tmax(g, scale = FALSE)
Condition
Warning:
The `scale` argument of `centr_eigen_tmax()` is deprecated as of igraph 2.1.3.
i The function always behaves as if `scale` were TRUE. The argument will be removed in the future.

# centr_eigen() deprecated argument

Code
c <- centr_eigen(g, scale = FALSE)
Condition
Warning:
The `scale` argument of `centr_eigen()` is deprecated as of igraph 2.1.3.
i The function always behaves as if `scale` were TRUE. The argument will be removed in the future.

# centr_degree_tmax() deprecated argument

Code
c <- centr_degree_tmax(g)
Condition
Warning:
The `loops` argument of `centr_degree_tmax()` must be explicit as of igraph 2.0.0.
i Default value (`FALSE`) will be dropped in next release, add an explicit value for the loops argument.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually wonder why an explicit value is needed here.

Copy link
Member

Choose a reason for hiding this comment

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

There is no reasonable default. The user should be forced to make a decision. The decision can only be made based on an understanding of where the network came from—it can't be deduced by looking at the network structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooooh I understand now. I think the current message is slightly misleading: I thought support for TRUE was getting dropped, not support for not choosing. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But now that I know what it means I am not sure how to rephrase the message.


47 changes: 47 additions & 0 deletions tests/testthat/test-centralization.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
test_that("centr_eigen_tmax() works", {
withr::local_seed(42)
g <- sample_pa(1000, m = 4)
expect_equal(centr_eigen_tmax(g), 998)
})

test_that("centr_eigen_tmax() deprecated argument", {
g <- sample_pa(1000, m = 4)
expect_snapshot(c <- centr_eigen_tmax(g, scale = FALSE))
})

test_that("centr_eigen() works", {
withr::local_seed(42)
g <- sample_pa(1000, m = 4)
centr_eigen <- centr_eigen(g)
expect_setequal(
names(centr_eigen),
c("vector", "value", "options", "centralization", "theoretical_max")
)
expect_equal(centr_eigen$centralization, 0.9432924, tolerance = 1e-06)
})

test_that("centr_eigen() deprecated argument", {
g <- sample_pa(1000, m = 4)
expect_snapshot(c <- centr_eigen(g, scale = FALSE))
})

test_that("centr_degree_tmax() works", {
withr::local_seed(42)
g <- sample_pa(1000, m = 4)
expect_gt(centr_degree_tmax(g, loops = TRUE), 1990000)
})

test_that("centr_degree_tmax() deprecated argument", {
g <- sample_pa(1000, m = 4)
expect_snapshot(c <- centr_degree_tmax(g))
})

test_that("centr_betw() works", {
withr::local_seed(42)
g <- sample_pa(1000, m = 4)
expect_setequal(
names(centr_betw(g)),
c("res", "centralization", "theoretical_max")
)
expect_equal(centr_betw(g)$theoretical_max, 996004998)
})
Loading