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

deduplication of louvain and leiden doc #570

Open
fbnrst opened this issue Mar 29, 2019 · 5 comments
Open

deduplication of louvain and leiden doc #570

fbnrst opened this issue Mar 29, 2019 · 5 comments
Labels
Area - Documentation 📒 Enhancement ✨ good first issue easy first issue to get started in OSS community contribution!

Comments

@fbnrst
Copy link
Contributor

fbnrst commented Mar 29, 2019

louvain and leiden have a lot of redundant documentation. After having learned in #557, I could file a PR to deduplicate this. Would it be valid to shuffle the arguments in such a way that the shared documentation is grouped together? Otherwise, one would have to introduce many short strings and puzzle them together.

@flying-sheep
Copy link
Member

flying-sheep commented Mar 29, 2019

Sounds like a great idea. generally the order should be the same as in the signature, but I don’t see a problem in reshuffling the lovain args to match the leiden ones.

We have to be careful with details though: e.g. partition_type needs to be slightly different for both:

Type of partition to use. Defaults to :class:`~louvain.RBConfigurationVertexPartition`.
For the available options, consult the documentation for :func:`~louvain.find_partition`.
Type of partition to use. Defaults to :class:`~leidenalg.RBConfigurationVertexPartition`.
For the available options, consult the documentation for :func:`~leidenalg.find_partition`.

@falexwolf do you think we should go ahead with https://pypi.org/project/legacy-api-wrap (and introduce * in louvain’s signatureor do you think we can slightly reshuffle the last few arguments oflouvain` without considering it a backwards compat break?

@gokceneraslan
Copy link
Collaborator

Wouldn't it actually make sense to merge louvain and leiden into one function and add an algorithm option or so?

@flying-sheep
Copy link
Member

flying-sheep commented Mar 29, 2019

I think we talked about that at one point. But yes, makes a lot of sense. detect_communities would be accurate, but long. Maybe just partition or partitioning?

@LuckyMD
Copy link
Contributor

LuckyMD commented Mar 29, 2019

maybe sc.tl.modularity_clustering() so that you keep the option open of adding other clustering functions in the future. Not exactly shorter though ;).

@ivirshup
Copy link
Member

@gokceneraslan since they are largely the same thing (just a different optimization strategy), do we even need to keep both?

Otherwise, I think I'd prefer them to be separate functions, so you don't get argument interactions. For example, the partition_type argument has to be a type from the same package as the method, otherwise there are segfaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area - Documentation 📒 Enhancement ✨ good first issue easy first issue to get started in OSS community contribution!
Projects
None yet
Development

No branches or pull requests

5 participants