refactor: use {lifecycle} for deprecation #1014

maelle · 2023-12-18T14:46:56Z

aviator-app · 2023-12-18T14:46:58Z

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 pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.

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.

revdep/problems.md

krlmlr · 2023-12-29T05:13:20Z

revdep/problems.md

+    ℹ Please use `set_edge_attr()` instead.
+    > causal.effect("y", "x", G = g)
+    Error in simple_es_index(x, ii) : Unknown edge selected
+    Calls: causal.effect ... [ -> [.igraph.es -> lapply -> FUN -> simple_es_index


No idea why this is failing now.

krlmlr · 2023-12-29T05:13:41Z

revdep/problems.md

+       3. │   └─rlang::eval_bare(expr, quo_get_env(quo))
+       4. └─cfid::import_graph(ig)
+       5.   └─cfid::dag(paste0(c(g_obs, g_unobs), collapse = "; "))
+       6.     └─cfid:::stopifnot_(nzchar(x), "Argument `x` contains only whitespace or special characters.")


revdep/problems.md

krlmlr · 2023-12-29T05:14:27Z

revdep/problems.md

+    > q.arg = list(list(min=-pi, max=pi), list(min=-pi, max=pi), list(min=-pi, max=pi)) 
+    > estimateGraph(f.mat=ishigami.fun, d=3, q.arg=q.arg, n.tot=10000, method="LiuOwen")
+    Error in (function (edges, n = max(edges), directed = TRUE)  : 
+      unused arguments (3, FALSE)


again related to how we pass the ... 👀

now I'm wondering whether there are uncaught occurrences of the same issue for other deprecated functions, I'll check.

although, in this case, I'm starting to think the error is one usage error that existed before but wasn't caught.

The code is graph(as.vector(t(E)), d , FALSE) where d is 3 and as.vector(t(E)) is c(1, 3). In make_graph(), the ... are for "when the graph is given via a literal". Here the edges are numeric. So we're in

rigraph/R/make.R

Lines 612 to 627 in b806ec2

old_graph <- function(edges, n = max(edges), directed = TRUE) {

on.exit(.Call(R_igraph_finalizer))

if (missing(n) && (is.null(edges) || length(edges) == 0)) {

n <- 0

}

.Call(

R_igraph_create, as.numeric(edges) - 1, as.numeric(n),

as.logical(directed)

)

}

args <- list(edges, ...)

if (!missing(n)) args <- c(args, list(n = n))

if (!missing(directed)) args <- c(args, list(directed = directed))

do.call(old_graph, args)

where the ... are actually not used since the old_graph() function doesn't have more arguments.

although, why did it used to work

igraph::make_graph(c(1, 3), 3, FALSE) #> IGRAPH 628f020 U--- 3 1 -- #> + edge from 628f020: #> [1] 1--3 igraph::graph(c(1, 3), 3, FALSE) #> Warning: `graph()` was deprecated in igraph 2.0.0. #> ℹ Please use `make_graph()` instead. #> This warning is displayed once every 8 hours. #> Call `lifecycle::last_lifecycle_warnings()` to see where this warning was #> generated. #> Error in (function (edges, n = max(edges), directed = TRUE) : unused arguments (3, FALSE)

^{Created on 2024-01-08 with reprex v2.0.2}

krlmlr · 2023-12-29T05:14:48Z

revdep/problems.md

+
+    Quitting from lines 46-56 [unnamed-chunk-5] (Tutorial.Rmd)
+    Error: processing vignette 'Tutorial.Rmd' failed with diagnostics:
+    At vendor/cigraph/src/graph/type_indexededgelist.c:101 : Number of vertices must not be negative. Invalid value


krlmlr · 2023-12-29T05:26:00Z

R/make.R

+  }
+
+  if (missing(simplify)) {
+    make_graph(edges = edges, n = n, isolates = isolates, directed = directed, ...)


Would this fail if edges is a formula?

I don't like the interface of make_graph(), but deprecating it is an uphill battle. Can we instead export all relevant functions (e.g., make_famous_graph()), and supersede it?

let's open another issue regarding make_graph()'s superseding, and I'll do my best to fix the current graph() here in this PR.

maelle · 2024-01-08T14:56:28Z

I wonder how, in the problematic cases, we could "replay" the call exactly, just switching the function name. https://rlang.r-lib.org/reference/stack.html

maelle · 2024-01-08T15:02:00Z

I'll come back to this later (this week or next), still some thinking needed.

maelle · 2024-01-12T14:40:23Z

@krlmlr we could discuss the approach in b33124c (#1014)

maelle · 2024-01-12T14:41:03Z

The functions needing a similar fix are graph() but also graph.famous() and graph.formula(). 🤔

…ph() function is rather lang

maelle requested a review from krlmlr December 18, 2023 14:46

aviator-app bot force-pushed the zzz-december branch from 483b08a to 779c868 Compare December 19, 2023 01:39

krlmlr force-pushed the zzz-december branch from e941a09 to 1fbc9f7 Compare December 25, 2023 09:57

krlmlr mentioned this pull request Dec 25, 2023

chore: Add tools script and template for moving to lifecycle #1022

Merged

krlmlr force-pushed the zzz-december branch 2 times, most recently from e22d6fe to 5e3bdfa Compare December 26, 2023 17:25

refactor!: change R/make.R

3d4b9c7

krlmlr force-pushed the zzz-december branch from 5e3bdfa to 8fe0b0d Compare December 29, 2023 05:04

krlmlr reviewed Dec 29, 2023

View reviewed changes

maelle mentioned this pull request Jan 8, 2024

Check whether more deprecated functions are passing ... wrongly #1088

Open

maelle mentioned this pull request Jan 12, 2024

Supersede make_graph()? #1103

Open

maelle mentioned this pull request Jan 12, 2024

refactor!: change R/par.R #1104

Merged

maelle and others added 4 commits January 12, 2024 15:54

refactor: remove the deprecated() function

d0bc9dd

Results

32e8283

fix graph.formula() (#6)

3a3d10b

try a strategy of call 'hacking' rather than inlining as the make_gra…

67fa6da

…ph() function is rather lang

maelle force-pushed the zzz-december branch from b33124c to 67fa6da Compare January 12, 2024 14:55

maelle mentioned this pull request Jan 15, 2024

Rename resolution_parameter to resolution #1015

Open

krlmlr marked this pull request as draft April 9, 2024 13:43

krlmlr assigned krlmlr and maelle and unassigned krlmlr Apr 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use {lifecycle} for deprecation #1014

refactor: use {lifecycle} for deprecation #1014

maelle commented Dec 18, 2023 •

edited

aviator-app bot commented Dec 18, 2023

krlmlr Dec 29, 2023

krlmlr Dec 29, 2023

krlmlr Dec 29, 2023

maelle Jan 8, 2024

maelle Jan 8, 2024

maelle Jan 8, 2024

maelle Jan 8, 2024

maelle Jan 8, 2024

krlmlr Dec 29, 2023

krlmlr Dec 29, 2023

maelle Jan 12, 2024

maelle commented Jan 8, 2024 •

edited

maelle commented Jan 8, 2024

maelle commented Jan 12, 2024

maelle commented Jan 12, 2024

	old_graph <- function(edges, n = max(edges), directed = TRUE) {
	on.exit(.Call(R_igraph_finalizer))
	if (missing(n) && (is.null(edges) \|\| length(edges) == 0)) {
	n <- 0
	}
	.Call(
	R_igraph_create, as.numeric(edges) - 1, as.numeric(n),
	as.logical(directed)
	)
	}

	args <- list(edges, ...)
	if (!missing(n)) args <- c(args, list(n = n))
	if (!missing(directed)) args <- c(args, list(directed = directed))

	do.call(old_graph, args)

refactor: use {lifecycle} for deprecation #1014

Are you sure you want to change the base?

refactor: use {lifecycle} for deprecation #1014

Conversation

maelle commented Dec 18, 2023 • edited

aviator-app bot commented Dec 18, 2023

Current Aviator status

How to merge

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maelle commented Jan 8, 2024 • edited

maelle commented Jan 8, 2024

maelle commented Jan 12, 2024

maelle commented Jan 12, 2024

maelle commented Dec 18, 2023 •

edited

maelle commented Jan 8, 2024 •

edited