feat: remove deprecated functions before 1.0 #1352

Antonov548 · 2024-05-02T07:34:59Z

Deprecated

igraph_bipartite_game() — see Split sample_bipartite() into two functions for the G(n,m) and G(n,p) case #630 - not use at all

Updated

igraph_hub_score() - replaced with igraph_hub_and_authority_scores(), see Avoid using deprecated functions from igraph/C #1177 (comment)
igraph_authority_score() - replaced with igraph_hub_and_authority_scores(), see Avoid using deprecated functions from igraph/C #1177 (comment)
igraph_lattice() — replace with igraph_square_lattice(), see Update make_lattice() / lattice() #994
igraph_laplacian() — replace by igraph_get_laplacian()
igraph_erdos_renyi_game() — not used in R.
igraph_random_edge_walk() — functionality merged into igraph_random_walk().

R side

hub_score() - added hits_scores. hub_score is deprecated and use hits_scores under the hood.
authority_score() - added hits_scores. authority_score is deprecated and use hits_scores under the hood.
make_lattice() - use square_lattice(). circular parameter is deprecated. Added periodic parameter. periodic should be logical vector with same length as dim or single value which will be converted to vector.
laplacian_matrix - use generated get_laplacian_sparse_impl and get_laplacian_impl instead of R_igraph_laplacian. normalized argument is deprecated with new normalization argument.
igraph_erdos_renyi_game() — not used in R.
igraph_random_edge_walk() — updated random_edge_walk to use random_walk_impl with edges return.

aviator-app · 2024-05-02T07:35:03Z

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.

Antonov548 · 2024-05-02T07:35:46Z

@krlmlr
This is related to igraph/igraph#2575 (comment)

Is it right that we should just remove deprecated functions?

szhorvat · 2024-05-02T08:56:17Z

I edited the top post and added information on what can / should be done.

szhorvat · 2024-05-02T08:57:50Z

NAMESPACE

@@ -483,8 +481,7 @@ export(hrg.fit)
 export(hrg.game)
 export(hrg.predict)
 export(hrg_tree)
-export(hub.score)
-export(hub_score)
+export(hub_and_authority_scores)


I suggest hits_scores() here, see comment I linked

Antonov548 · 2024-05-02T09:02:20Z

I edited the top post and added information on what can / should be done.

Thanks!

szhorvat · 2024-05-02T09:35:39Z

Let me know if you need input here, for example on what parameters to set in the new function to get the behaviour of the old one.

Regarding hub and authority scores, I expect that removing these without deprecation would cause quite the uproar. What could be done is to implement hub_score() and authority_score() in pure R in terms of hits_scores() and keep them deprecated for a while.

There will also be another change in these two functions, as well as eigen_centrality() in 1.0: the scale parameter will be removed, and the function will always behave as if scale=TRUE was set. I'll open a new issue for preparing for this change, but I suggest NOT to expose the scale parameter at all when introducing hits_scores().

Antonov548 · 2024-05-03T11:22:19Z

@szhorvat

I think we still need to remove hub_score() and authority_score() because implementing same functions, but using hits_scores() still break interface on R side, so maybe it worth just to remove them.
Should then functions which are using igraph_get_laplacian to be extended with additional argument mode?

szhorvat · 2024-05-03T11:38:07Z

I think we still need to remove hub_score() and authority_score() because implementing same functions, but using hits_scores() still break interface on R side, so maybe it worth just to remove them.

Why would it break the interface? hits_scores() returns both scores. Just drop one and rewrite the result to the format that hub_score() used to return.

As for the scale parameter, allow the parameter but ignore its value. We never made any promises about the type of scaling used with scale=FALSE.

Antonov548 · 2024-05-03T11:46:12Z

I think we still need to remove hub_score() and authority_score() because implementing same functions, but using hits_scores() still break interface on R side, so maybe it worth just to remove them.

Why would it break the interface? hits_scores() returns both scores. Just drop one and rewrite the result to the format that hub_score() used to return.

As for the scale parameter, allow the parameter but ignore its value. We never made any promises about the type of scaling used with scale=FALSE.

Because hub_score() and authority score() returns not just a vector. Of course we can maybe try to keep previous behavior by creating the similar list.
https://github.com/igraph/rigraph/pull/1352/files#diff-aa8edbd6189f5557d42166c470f065672ba52363e61d84dcd7bd3200c3b3420cL3860-L3862

Sorry, I didn't get regarding laplacian. It has also mode=c("out", "in", "all", "total") and igraph_laplacian hasn't this argument.

szhorvat · 2024-05-03T11:49:01Z

Should then functions which are using igraph_get_laplacian to be extended with additional argument mode?

This will require a somewhat breaking change (which is why I kept pushing to do this for 2.0...)

The big change is not mode but the normalization parameter, which is NOT the same as normalized. The old igraph_laplacian() normalized in an inconsistent way. You can see what it did here:

https://github.com/igraph/igraph/blob/223fd0e2c11b4c0a35452ffa68347206771dc0b4/src/properties/spectral.c#L403-L431

We need to transition from the old boolean normalized to the new normalization in some way. I'm not sure what the customary way is in R. I guess we can temporarily have both normalized and normalization at the same time. Deprecate normalized, i.e. any explicit use of normalized should trigger a deprecation warning. I'm not sure what to do when the user passes both normalized and normalization at the same time: I guess throw an error since this makes no sense?

szhorvat · 2024-05-03T11:52:43Z

Re hub and authority scores:

Currently the function returns a list with many named elements. The main result is in $vector. hits_scores() would return the same, except that instead of vector it'd have both $hub and $authority. hub_score() would call hits_scores(), would remove $authority and would rename $hub to $vector. The rest of the entries are the same between the two functions.

Is there anything I'm missing that may continue to be a problem?

szhorvat · 2024-05-08T08:50:57Z

I notice you were working on make_lattice. Please see this before going ahead: #994

The R interface shouldn't always directly mirror the raw C interface. The periodic parameter should accept either a single logical value or a vector. The circular parameter probably shouldn't be removed immediately, breaking compatibility, but should go through a gradual deprecation.

Antonov548 · 2024-05-08T08:55:49Z

I notice you were working on make_lattice. Please see this before going ahead: #994

The R interface shouldn't always directly mirror the raw C interface. The periodic parameter should accept either a single logical value or a vector. The circular parameter probably shouldn't be removed immediately, breaking compatibility, but should go through a gradual deprecation.

Sure. I think derpecation of old parameter name is quite easy.

Can you clarify please about single logical value or a vector? You mean it should be handled on R side?
Because in core as I understood it could be NULL or vector with exact same length as dimension.

szhorvat · 2024-05-08T09:02:24Z

Can you clarify please about single logical value or a vector? You mean it should be handled on R side?
Because in core as I understood it could be NULL or vector with exact same length as dimension.

Yes, it should be handled in R. I would not allow NULL at all. Instead:

it's either a logical vector of the same length as the dimension (which can be sent directly to C)
or a logical scalar, which will be replicated dimension times (which can't be implemented in C, so needs to be handled in R)

The default should be FALSE.

This is both very convenient (in most cases people will pass a scalar only) and it is compatible with how the old circular parameter behaved.

szhorvat · 2024-05-08T11:10:27Z

In the top post I linked to relevant issues where I tried to describe what's needed for the non-trivial upgrades. If anything's unclear (as in this case) please ask :-)

R/make.R

Antonov548 · 2024-05-22T13:54:43Z

@maelle
May I ask you to help with documentation in this PR? I trying to update documentation and have error:

✖ In topic 'authority_score.Rd': Skipping; no name and/or title.
✖ In topic 'hub_score.Rd': Skipping; no name and/or title.

I don't get what is missing in documentation, mentioned functions have rdname.

maelle · 2024-05-23T14:09:10Z

I'll come back to this next week!

maelle · 2024-05-28T11:10:22Z

Do we really want to remove the functions though? Shouldn't they be deprecated first?

NAMESPACE

R/centrality.R

R/games.R

maelle · 2024-05-28T11:27:17Z

Above authority_score and hub_score you need

#' @title Kleinberg's hub and authority centrality scores.

krlmlr

Can you please review with Maëlle?

R/centrality.R

krlmlr · 2024-05-30T07:37:07Z

For the test failure, the problem and solution pattern could look like this:

wrapped <- function(a, b, c) {
  list(a, b, c)
}

fun <- function(..., b = NULL, c = NULL) {
  wrapped(..., a = 5, b = b, c = c)
}

fun(b = 2, c = 3)
#> [[1]]
#> [1] 5
#> 
#> [[2]]
#> [1] 2
#> 
#> [[3]]
#> [1] 3

fun(a = 1, b = 2, c = 3)
#> Error in wrapped(..., a = 5, b = b, c = c): formal argument "a" matched by multiple actual arguments

fun <- function(..., a = NULL, b = NULL, c = NULL) {
  a <- if (is.null(a)) 5 else a
  wrapped(..., a = a, b = b, c = c)
}

fun(a = 1, b = 2, c = 3)
#> [[1]]
#> [1] 1
#> 
#> [[2]]
#> [1] 2
#> 
#> [[3]]
#> [1] 3

^{Created on 2024-05-30 with reprex v2.1.0}

maelle · 2024-05-30T08:38:30Z

@Antonov548 I'll check out this PR locally and make the changes

no longer removing the functions
doing the argument adaptation mentioned above by Kirill
...

Antonov548 · 2024-05-30T08:40:36Z

@Antonov548 I'll check out this PR locally and make the changes

no longer removing the functions

doing the argument adaptation mentioned above by Kirill

...

Thanks. I have restored functions. Just pushed changes.
Yes, please, take a look.

szhorvat · 2024-05-30T08:56:29Z

In sample_gnm() / sample_gnp(), the type1 and type variables can now be removed. type1 is not used and the value of type can be inlined, simplifying code.

szhorvat · 2024-06-06T13:25:16Z

Or better: use macaque from igraphdata.

maelle · 2024-06-07T06:22:05Z

@Antonov548 I do not understand why aaa-auto.R refers to hub.vector. How exactly is the body of hub_and_authority_scores_impl() created? In functions-R.yaml I only see

igraph_hub_and_authority_scores:
    # This is a temporary hack; we need to find a way to handle default values
    # for dependencies. The problem is that the VERTEXINDEX type needs two
    # dependencies: the graph and the vertex set, but we only have the graph
    # in the argument list.
    DEPS: weights ON graph, hub_vector ON graph V(graph), authority_vector ON graph V(graph)

How do we best patch hub_and_authority_scores_impl()? It should refer to names with underscores not with dots. Thank you!

maelle · 2024-06-07T06:35:03Z

The example I tried adding pairs as a reprex of what's broken @Antonov548

library("igraphdata")
library("igraph")
#> 
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#> 
#>     decompose, spectrum
#> The following object is masked from 'package:base':
#> 
#>     union

data(macaque)
hits_scores(macaque)
#> This graph was created by an old(er) igraph version.
#>   Call upgrade_graph() on it to use with the current igraph version
#>   For now we convert it on the fly...
#> Error in names(res$hub.vector) <- vertex_attr(graph, "name", V(graph)): attempt to set an attribute on NULL

^{Created on 2024-06-07 with reprex v2.1.0}

Antonov548 · 2024-06-07T07:58:31Z

@Antonov548 I do not understand why aaa-auto.R refers to hub.vector. How exactly is the body of hub_and_authority_scores_impl() created? In functions-R.yaml I only see
igraph_hub_and_authority_scores:
    # This is a temporary hack; we need to find a way to handle default values
    # for dependencies. The problem is that the VERTEXINDEX type needs two
    # dependencies: the graph and the vertex set, but we only have the graph
    # in the argument list.
    DEPS: weights ON graph, hub_vector ON graph V(graph), authority_vector ON graph V(graph)
How do we best patch hub_and_authority_scores_impl()? It should refer to names with underscores not with dots. Thank you!

To not spend time to make changes in stimulus I can propose following change. Please check last commit.

I found a place in stimulus where _ is replaced by dot. Seems this logic not synchronized with C code generation.

Antonov548 · 2024-06-10T14:35:33Z

He... I'm not sure what with GHA here. Maybe something wrong with cache. @krlmlr may you suggest please?

krlmlr · 2024-06-11T16:50:48Z

I don't see a caching step, can you please be more specific?

Antonov548 · 2024-06-11T16:54:42Z

I don't see a caching step, can you please be more specific?

I mean this caches. I already had once the problem with them - https://github.com/igraph/rigraph/actions/caches.
But as I know we don't have easy way to clear all of them.

Antonov548 · 2024-06-12T09:12:28Z

@krlmlr the issue wasn't the cache. I had some troubles with search in VS code, so that I didn't see another usage of removed function. But it's good right now and maybe it make sense to run check for packages once again.

krlmlr · 2024-06-16T14:33:14Z

This looks green. Running revdepchecks.

krlmlr · 2024-06-16T17:34:21Z

Looks good enough to me?

Antonov548 · 2024-06-16T18:21:32Z

Looks good enough to me?

Also looks good to me. Thanks for checking it.

szhorvat · 2024-06-16T18:36:00Z

I'm on my phone and can't look properly, but this caught my eye:

Error in igraph::graph.lattice(length(subwords) + 1, directed = TRUE) : 
  argument "circular" is missing, with no default

This is changing a deprecated function, which is perhaps not a good idea?

The circular / periodic parameters are otherwise not mandatory, right?

szhorvat · 2024-06-16T18:37:47Z

The netropy issue is not our problem.

Did anyone look and check that the tidygraph one also isn't?

krlmlr · 2024-06-16T19:19:57Z

tidygraph is present in main, I think this was also something that we can/should fix, in addition to the circular argument.

@maelle: can you please take a closer look?

This isn't meant to be a blocker for this PR, just something that we want to do before release.

szhorvat · 2024-06-16T19:37:23Z

This isn't meant to be a blocker for this PR, just something that we want to do before release.

Absolutely!

szhorvat · 2024-06-16T19:41:01Z

make_lattice should also be renamed make_square_lattice to make room for make_hex_lattice() and make_tri_lattice() (or whatever you want to name them). #994

krlmlr · 2024-06-18T07:05:08Z

Thanks!

Antonov548 changed the title ~~chore: remove deprecated functions before 1.0~~ feat: remove deprecated functions before 1.0 May 2, 2024

szhorvat reviewed May 2, 2024

View reviewed changes

szhorvat reviewed May 9, 2024

View reviewed changes

R/make.R Outdated Show resolved Hide resolved

maelle added this to the 2.0.4 milestone May 23, 2024

maelle added the hackathon-topic 📆 label May 28, 2024

maelle reviewed May 28, 2024

View reviewed changes

krlmlr reviewed May 30, 2024

View reviewed changes

R/centrality.R Show resolved Hide resolved

R/centrality.R Show resolved Hide resolved

krlmlr removed the hackathon-topic 📆 label May 30, 2024

rename params

4da1aac

Antonov548 added 2 commits June 10, 2024 10:35

Merge branch 'main' into fix/deprecated

b10939c

update cpp11

26e8ecc

krlmlr and others added 3 commits June 11, 2024 18:55

chore: Bump

bc3e903

remove R_igraph_lattice

900b490

update docs

d61dc0a

Antonov548 and others added 3 commits June 13, 2024 13:32

Merge remote-tracking branch 'origin/main' into fix/deprecated

ec1aa2c

update interface

c2f4d1b

Merge branch 'main' into fix/deprecated

c338cd9

Results

b974789

szhorvat mentioned this pull request Jun 16, 2024

Update make_lattice() / lattice() #994

Open

2 tasks

krlmlr merged commit 865925b into main Jun 18, 2024
14 of 19 checks passed

krlmlr deleted the fix/deprecated branch June 18, 2024 07:05

feat: remove deprecated functions before 1.0 #1352

feat: remove deprecated functions before 1.0 #1352

Conversation

Antonov548 commented May 2, 2024 • edited Loading

Deprecated

Updated

R side

aviator-app bot commented May 2, 2024 • edited Loading

Current Aviator status

Antonov548 commented May 2, 2024

szhorvat commented May 2, 2024

szhorvat May 2, 2024

Choose a reason for hiding this comment

Antonov548 commented May 2, 2024

szhorvat commented May 2, 2024 • edited Loading

Antonov548 commented May 3, 2024

szhorvat commented May 3, 2024 • edited Loading

Antonov548 commented May 3, 2024

szhorvat commented May 3, 2024

szhorvat commented May 3, 2024 • edited Loading

szhorvat commented May 8, 2024

Antonov548 commented May 8, 2024

szhorvat commented May 8, 2024 • edited Loading

szhorvat commented May 8, 2024

Antonov548 commented May 22, 2024

maelle commented May 23, 2024

maelle commented May 28, 2024

maelle commented May 28, 2024

krlmlr left a comment

Choose a reason for hiding this comment

krlmlr commented May 30, 2024

maelle commented May 30, 2024

Antonov548 commented May 30, 2024

szhorvat commented May 30, 2024

szhorvat commented Jun 6, 2024

maelle commented Jun 7, 2024

maelle commented Jun 7, 2024

Antonov548 commented Jun 7, 2024

Antonov548 commented Jun 10, 2024 • edited Loading

krlmlr commented Jun 11, 2024

Antonov548 commented Jun 11, 2024

Antonov548 commented Jun 12, 2024

krlmlr commented Jun 16, 2024

krlmlr commented Jun 16, 2024

Antonov548 commented Jun 16, 2024

szhorvat commented Jun 16, 2024

szhorvat commented Jun 16, 2024

krlmlr commented Jun 16, 2024

szhorvat commented Jun 16, 2024

szhorvat commented Jun 16, 2024

krlmlr commented Jun 18, 2024

Antonov548 commented May 2, 2024 •

edited

Loading

aviator-app bot commented May 2, 2024 •

edited

Loading

szhorvat commented May 2, 2024 •

edited

Loading

szhorvat commented May 3, 2024 •

edited

Loading

szhorvat commented May 3, 2024 •

edited

Loading

szhorvat commented May 8, 2024 •

edited

Loading

Antonov548 commented Jun 10, 2024 •

edited

Loading