kharchenkolab/leidenAlg

Include other communities to the repo

VPetukhov opened this issue · 18 comments

VPetukhov commented

How about making it broader and including all communities from Conos here? I'm a particularly big fan of the rleiden community.

evanbiederstedt commented

I think this is is a very good idea.

Questions:
--- Why were these methods deprecated? e.g. https://github.com/hms-dbmi/conos/blob/master/R/communities.R#L15
--- Could you explain this TODO comment a bit more? https://github.com/hms-dbmi/conos/blob/master/R/communities.R#L1

evanbiederstedt commented

Hi @VPetukhov

Ok, I added the "community functions". I'll wait for the discussion about questions above, and then we can close this.

pkharchenko commented
evanbiederstedt commented

I removed multimulti.community() and multitrap.community(), and renamed a few functions for clarity, i.e.

  • leiden.community() -> leiden.community.detection() (as the name was close to the Rcpp function, leiden_community())
  • as.dendrogram.fakeCommunities() -> dendrogram.fakeCommunities() (as this name reflects the pattern of membership.fakeCommunities(), and as.dendrogram.fakeCommunities() was giving me roxygen2 problem, i.e. roxygen2 would incorrectly deduce the function as an S3 method, as igraph::as.dendrogram() exists)

A few issues:

  • Is fakeCommunities a good class name? I don't think it's so bad
  • The functions ``leiden_community()andleiden.community.detection()` are pretty similar in scope.

EDIT: Also, I'm not sure it's worthwhile to keep the wrapper functions dendrogram.fakeCommunities() and membership.fakeCommunities(), especially as rleiden.community() doesn't return a dendrogram.

Let me know your thoughts, CC @VPetukhov @pkharchenko

pkharchenko commented
evanbiederstedt commented

leiden.community() was named to be consistent with the rest of the *.communitu() igraph functions (e.g. multilevel.community, walktrap.community, etc.) … also, it’s used in a lot of code, so it’s probably best to avoid renaming that one.

Fair enough. How about changing leiden_community() to find_partition()? That's roughly how the python interface works (and it's very close to the same design).

The question is really how leiden_community() and leiden.community() could both nicely exist in the same package.

pkharchenko commented
evanbiederstedt commented

I exported it with fancy Rcpp documentation to be sure :) https://github.com/kharchenkolab/leidenAlg/blob/master/src/leiden.cpp#L43-L50

In that case, I'll rename, and remove from the user.

EDIT: Remaining issues to decide

  • Is fakeCommunities a good class name? I don't think it's so bad
  • Also, I'm not sure it's worthwhile to keep the wrapper functions dendrogram.fakeCommunities() and membership.fakeCommunities(), especially as rleiden.community() doesn't return a dendrogram.
VPetukhov commented

Is fakeCommunities a good class name? I don't think it's so bad

Can we use some default name from igraph? I'm not sure we need another class for this.

especially as rleiden.community() doesn't return a dendrogram.

Hm, it does: dendrogram=combd, doesn't it?

evanbiederstedt commented

Can we use some default name from igraph? I'm not sure we need another class for this.

I'm open to suggestions. I wasn't sure if the "fakeCommunities" class was used elsewhere.

Hm, it does: dendrogram=combd, doesn't it?

Fair enough, I became confused by the comment: https://github.com/hms-dbmi/conos/blob/master/R/communities.R#L203

if(hierarchical) {
    # calculate hierarchy on the multilevel clusters
    if(length(wtl)>1) {
        ....
        combd <- glue.dends(d)
    } else {
        combd <- as.dendrogram(wtl[[1]]);
    }
} else {
    combd <- NULL;
}

res <- list(membership=fv, dendrogram=combd, algorithm='rleiden', names=names(fv))
pkharchenko commented
evanbiederstedt commented

Can we use some default name from igraph? I'm not sure we need another class for this.

I think this is a laudable goal, but the downside of using an actual class name from R's igraph would be that, we would need to make sure that rleiden.community and our other code is totally compatible with this igraph class, and I'm not sure that would be worth the effort tbh. (I would re-consider if there was some user requests, or better reason for this needed.)

Probably the closest class for an output is communities; at least the igraph class as input doesn't work out of the box: https://igraph.org/r/doc/communities.html

library(igraph)
karate <- make_graph("Zachary")
print(class(karate))  ##  "igraph"
wc <- cluster_walktrap(karate) ## "communities"
conos::rleiden.community(karate)
## Error in leiden_community(graph, E(graph)$weight, resolution, n.iterations) : 
##   Not compatible with requested type: [type=NULL; target=double].

I think it’s used in rleiden ... but it’s an internal class, so I think we can name it anything we want. It’s just to return something that looks like an igraph community return.

Yes, I like the idea, and I understood why it's called "fakeCommunities". I think my sense of aesthetics just wanted a better name :)

pkharchenko commented
evanbiederstedt commented

Agreed. Any objections?

If "fakeCommunities" sounds off, "leidenCommunities" or something works. No biggy.

VPetukhov commented

If "fakeCommunities" sounds off, "leidenCommunities" or something works. No biggy.

I'm fine with "fakeCommunities". And I'd prefer something more general than "leidenCommunities", as we have it for all our internal communities.

evanbiederstedt commented

as we have it for all our internal communities.

Aren't the other communities all deprecated/not used?

VPetukhov commented

Aren't the other communities all deprecated/not used?

Currently, yes. But who knows what we will decide to implement :)

evanbiederstedt commented

Fair enough! We'll close for now.

If you have better names than "fake", let me know :)

  • 👍1