search

GuyAllard / markov_clustering

markov clustering in python
MIT License
168 stars 37 forks source link

Add modularity #6

Closed Moonire closed 6 years ago

Moonire commented 6 years ago

Addition of the modularity module to compute the quality of the clustering. Giving us the ability to choose the best values for our hyperparametres. Supports both dense and sparse matrices. Implementation in accordance with : Malliaros, Fragkiskos D., and Michalis Vazirgiannis. "Clustering and community detection in directed networks: A survey." Physics Reports 533.4 (2013): 95-142.

Note that the conversion step is costly and isn't perfect as there is no bijection between adjacency and transition matrices.

GuyAllard commented 6 years ago

Hi Moonire,

This is a great contribution! Thank you for implementing this and submitting the pull request.

I have a few small things that I would like to see added/changed :

  1. Your code currently uses tabs for indentation, I would like that to be changed to 4 spaces to be consistent with the rest of the module (and PEP 8).
  2. Some simple tests for the new functions (add these to tests/test_mcl.py or in a new file, e.g. tests/test_modularity.py).
  3. A module-level docstring for modularity.py which briefly describes the functionality. This will allow a description of your contribution to be included in the auto-generated read-the-docs documentation.

Thanks again for taking the time to work on this.

Guy

Moonire commented 6 years ago

It was my pleasure!

Your 2nd and 3rd remarks were already on my todo list so I hope to be done with them soon.

It's my 1st contribution to an open source project so I was excited to submit it as soon as it was working :sweat_smile:.

Also, the thanks goes to you for making such a great quality and very efficient code. It saved a job I was doing. Keep up the great work.

Mounir

Moonire commented 6 years ago

Hi Guy,

I've tackled the issues you pointed out and did some refactoring too so that it would be clearer. I've to get your feedback as soon as possible!

Thanks

GuyAllard commented 6 years ago

That looks good. All the tests pass! I was experimenting with it today - it is really useful for determining the best cluster inflation value to use.

I will create a new feature branch which contains your work, and will make some additions to the documentation. Once that Is complete, I will merge into master and create a new release.

Thanks!

GuyAllard commented 6 years ago

One addition request: I would like to add you to the list of authors in init.py. To do this, I need your (real) name!

Moonire commented 6 years ago

Thank you I'd really appreciate it ! Its : Mounir MALLEK

GuyAllard commented 6 years ago

I have updated the README, added you to the author list and added the modularity module to the auto docs.

Can you review the README.md on the Moonire-modularity branch and check that the 'Choosing Hyperparameters' section that I have added makes sense?

Thanks!

Once that is done, I think this is ready to merge into master.

Moonire commented 6 years ago

It makes perfect sense, the exemple is very clear. Maybe mentioning it the features part would be relevant too.

Apart from that, I also think it's ready to merge into master.

GuyAllard commented 6 years ago

Good idea, I will add it to the features.

Moonire commented 6 years ago

Just a question which is probably stupid. What is the usual thing to do with the repo I forked after it's merged ? Should I delete it ?

GuyAllard commented 6 years ago

You can delete it.

GuyAllard commented 6 years ago

I'm merging this branch into master, everything seems good. Thanks for your contribution!

GuyAllard commented 6 years ago

The updated package is now available on pypi for installation using pip. https://pypi.org/project/markov-clustering/