Skip to content

Identify deprecated functions #685

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

Closed
maelle opened this issue Feb 20, 2023 · 30 comments
Closed

Identify deprecated functions #685

maelle opened this issue Feb 20, 2023 · 30 comments
Labels
lifecycle Deprecating old APIs
Milestone

Comments

@maelle
Copy link
Contributor

maelle commented Feb 20, 2023

  • some with #' @keywords internal DONE
  • some listed in the internal section of the pkgdown reference DONE
  • some have "removed from igraph" in their docs DONE

Functions with dots in their name that are not deprecated (or there is no alternative)

(This is a community-edited section.)

  • get.edge.ids()
  • as.igraph()
  • ... ??
@maelle

This comment was marked as resolved.

@szhorvat

This comment was marked as outdated.

@maelle

This comment was marked as outdated.

@maelle

This comment was marked as resolved.

@maelle

This comment was marked as resolved.

@maelle

This comment was marked as resolved.

@maelle

This comment was marked as resolved.

@szhorvat

This comment was marked as resolved.

@maelle
Copy link
Contributor Author

maelle commented Mar 6, 2023

@szhorvat is as.igraph() really deprecated?

@ntamas
Copy link
Member

ntamas commented Mar 6, 2023

Hmm, maybe it's time to start a checklist? (I'll try to stick to the lifecycle terminology).

  • layout.reingold.tilford() is deprecated, the replacement is layout_as_tree(). I would recommend officially marking it as deprecated with a warning, and then removing it in a future version.
  • layout.fruchterman.reingold.grid() is deprecated; the grid-based FR layout is now provided by layout_with_fr(). Since the deprecation warning has already been in place for a while, I think we can safely remove the function now.
  • layout.spring() and layout.svd() are deprecated without replacement. Right now they call layout_with_fr() instead, but this function is not a replacement. Since the warning has already been in place for a while, I think we can safely remove the functions now.
  • layout.grid.3d()is now layout_on_grid(..., dim=3) and a deprecation warning has already been in place for a long while now. It's time to let go of the old function now.
  • handle_vertex_type_arg() is an internal helper function and it is not meant to be public. I propose hiding it from the docs but keeping it as is.
  • as.igraph(): I don't think that this function has ever been used seriously. The idea was to provide a generic function that other data types can implement to convert themselves to an appropriate igraph object that represents them to some extent. For instance, as.igraph.igraphHRG() allows an igraphHRG object (a hierarchical random graph model, which can incidentally be represented as a graph) to be converted to an igraph graph for plotting purposes. Since it's a generic function, there is a chance that other R packages have implemented this for their own data types. I wouldn't remove it and I would not mark it as deprecated either.
  • get.edge.ids(): not deprecated, but I think we should have a proper underscored name for it and deprecate the dotted name.
  • erdos.renyi.game(): as quoted above, it is deprecated; the replacements are sample_gnm() and sample_gnp(). An old alias of it is random.graph.game(), also deprecated. However, since there were no warnings for users of these functions, I propose marking as deprecated and adding a warning, but not removing them yet.
  • vcount() and ecount(): these are currently marked in the documentation as being replaced by gorder() and gsize(), but I think the old names are better. I propose keeping both. If we really want to, we can say that vcount() superseded gorder() and ecount() superseded gsize() (or vice version).
  • neighborhood() is currently in zzz-deprecate.R and it claims ego() as a replacement, but to be honest I like neighborhood() better; ego() is a term that's mostly used in social network analysis but nowhere else. neighborhood() is more generic. I propose declaring neighborhood() as the preferred variant and ego() as an alias to it.
  • All nexus.*() functions can be removed.
  • as.directed() and as.undirected() use the old dotted-style naming, but I think we should be consistent and use underscored names instead. We should mark the old names as deprecated and remove them in a future version.
  • as.dendrogram(), as.hclust() and as.matrix() also use dotted names, but they are generic functions declared in other packages (e.g., stats) so I guess we must keep them as is.
  • .igraph.progress() and .igraph.status() are internal functions and should not appear in the public docs.

That's all that I've found in the docs so far.

@szhorvat

This comment was marked as resolved.

@ntamas

This comment was marked as resolved.

@maelle

This comment was marked as outdated.

@ntamas

This comment was marked as outdated.

@maelle

This comment was marked as outdated.

@maelle maelle added the lifecycle Deprecating old APIs label Mar 13, 2023
@szhorvat

This comment was marked as resolved.

@ntamas

This comment was marked as resolved.

@szhorvat

This comment was marked as resolved.

@krlmlr

This comment was marked as resolved.

@maelle

This comment was marked as outdated.

@szhorvat

This comment was marked as resolved.

@krlmlr krlmlr added this to the upgrade-2 milestone Jan 13, 2024
@maelle

This comment was marked as resolved.

@maelle

This comment was marked as resolved.

@maelle
Copy link
Contributor Author

maelle commented Sep 19, 2024

Only one item to go 🎉

.igraph.progress() and .igraph.status() are internal functions and should not appear in the public docs.

I am waiting on #1507 so it might be next week.

@maelle
Copy link
Contributor Author

maelle commented Sep 19, 2024

@ntamas @szhorvat what's the use of .igraph.progress() and .igraph.status()? Are they used somehow for the C integration? I'm making a PR that makes them internal but I'm wondering whether they should be removed.

@maelle
Copy link
Contributor Author

maelle commented Sep 23, 2024

@ntamas @szhorvat friendly reminder about the above 😺

@szhorvat
Copy link
Member

They should not be removed. They are used for the progress and status reporting. These functions are called from C to communicate progress/status.

Example:

console()

This shows the console.

cluster_edge_betweenness(sample_gnm(1000,2000))

Now watch the progress bar.

I see some debugging messages as well, which don't come from the C core. If these are reported though igraph_status(), they shouldn't. Status messages are meant to be informative to users, not to developers doing debugging (@krlmlr @Antonov548).

@Antonov548
Copy link
Contributor

Antonov548 commented Sep 23, 2024

Status messages are meant to be informative to users, not to developers doing debugging (@krlmlr @Antonov548).

It was done when we were working on serialization of graph to give some idea for the user what happens under the hood.
Should we remove it at all or move it somewhere else?

@maelle
Copy link
Contributor Author

maelle commented Sep 23, 2024

Thanks both!

I'll let someone more knowledgeable than me answer @Antonov548.

I think everything related to deprecation in this issue was done so afterwards we can close it. 🎉

@krlmlr
Copy link
Contributor

krlmlr commented Sep 24, 2024

The status messages during deserialization are an edge case, let's keep them for now -- not important. We didn't see other status messages.

@maelle maelle closed this as completed Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle Deprecating old APIs
Projects
None yet
Development

No branches or pull requests

5 participants