GuyAllard / markov_clustering

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

markov_clustering, possible error in instructions for computing of modularity Q #24

Open paolotgithub opened 4 years ago

paolotgithub commented 4 years ago

In calculating modularity Q I get the following error in my script that uses the markov_clustering package:

Traceback (most recent call last):
  File "mcl_3_inflat_test.py", line 48, in <module>
    Q = mc.modularity(matrix=result, clusters=clusters)
  File "/usr/local/lib/python3.7/site-packages/markov_clustering/modularity.py", line 74, in modularity
    matrix = convert_to_adjacency_matrix(matrix)
  File "/usr/local/lib/python3.7/site-packages/markov_clustering/modularity.py", line 39, in convert_to_adjacency_matrix
    coeff = max( Fraction(c).limit_denominator().denominator for c in col )
TypeError: 'float' object is not iterable

Shouldn't

Q = mc.modularity(matrix=result, clusters=clusters)

as it reads in the instructions be instead:

Q = mc.modularity(matrix, clusters=clusters) ?

Or otherwise...?

Thanks a lot.

grgmiller commented 4 years ago

I also experienced this issue when running

for inflation in [i / 10 for i in range(15, 26)]:
    result = mc.run_mcl(matrix, inflation=inflation)
    clusters = mc.get_clusters(result)
    Q = mc.modularity(matrix=result, clusters=clusters)
    print("inflation:", inflation, "modularity:", Q)

If you remove the [0] from the following line, the code seems to work, but I'm not sure if that is functionally correct (i.e. if you're supposed to be plugging int the result matrix or the original matrix as @plttvmt suggested) https://github.com/GuyAllard/markov_clustering/blob/28787cf64ef06bf024ff915246008c767ea830cf/markov_clustering/modularity.py#L37

BerkeAltiparmak commented 1 year ago

Problem

I've noticed that this issue still exists when matrix is a numpy array, so I proceeded to confirm whether @grgmiller 's approach is functionally correct, as he shared his concerns. Here is my procedure to confirm it.

Procedure

  1. First, create a new file called modularity.py (or whatever name you want to give to it).
  2. Copy-paste the contents of the modularity.py file from https://github.com/GuyAllard/markov_clustering/blob/28787cf64ef06bf024ff915246008c767ea830cf/markov_clustering/modularity.py
  3. Make the changes as proposed by @grgmiller, which is removing [0] from the line col = matrix[:,i].T.tolist()[0]
  4. Create a new file to test if this approach actually works.
  5. You can use this code to test the approach: Screen Shot 2022-11-27 at 20 36 32
  6. You can compare the outputs and notice that the one that uses the original approach as a sparse matrix gives approximately equal result to this approach that works with numpy arrays. Screen Shot 2022-11-27 at 20 38 12

Conclusion

The solution provided by @grgmiller works for similarity matrices that are of numpy arrays type.

cpouchon commented 7 months ago

Dear all, how can you import modularity from your modularity.py file ? I give this error: 7 from itertools import permutations 9 from scipy.sparse import isspmatrix, dok_matrix, find ---> 10 from .mcl import sparse_allclose 12 def is_undirected(matrix): 13 """ 14 Determine if the matrix reprensents a directed graph 15 16 :param matrix: The matrix to tested 17 :returns: boolean 18 """

ImportError: attempted relative import with no known parent package

Thank you, best regards Charles