search

networkx / nx-parallel

A networkx backend that uses joblib to run graph algorithms in parallel.
BSD 3-Clause "New" or "Revised" License
34 stars 21 forks source link

feat ✨ : Add closeness centrality and floyd warshall feature #72

Open Fre0Grella opened 3 months ago

Fre0Grella commented 3 months ago

Hi, i'm marco, a students from the university of Bologna. For my final project i want to contribute this repository adding the closeness centrality and the floyd warshall algorithms for numpy. The closeness centrality use the newly implemented floyd warshall. The parallelized floyd warshall is a version of tiled FW referenced in the code.

Schefflera-Arboricola commented 3 months ago

@Fre0Grella nx-parallel algorithms receive a ParallelGraph object, so you need to convert it back to the nx.Graph object to run the usual graph object methods on it. That's why you'll see the following lines in all the nx-parallel algorithms

if hasattr(G, "graph_object"):
        G = G.graph_object

Hopefully this will make the tests pass.

Thanks!

Fre0Grella commented 3 months ago

@Schefflera-Arboricola Hi, thanks for the first advice, i hope you could help with this one too. Running the test i got an error that i don't understand; on the closeness.py file, when i use joblib Parallel to call the _closeness_measure function (that from a row of the adjacency matrix get the value of closeness for the node rappresented by the row) i get the error in the joblib code saying TypeError: 'numpy.float64' object is not callable. I tried changing the data format thinking that joblib doesn't support numpy data structure but he kept saying in this case TypeError: 'float' object is not callable.

Schefflera-Arboricola commented 3 months ago

Hi @Fre0Grella, Thank you for your contribution :)

Is this PR ready for review? I will aim to get to it as soon as possible, though I cannot provide a specific timeline at the moment. I'm also tagging @dschult in case he would like to review it.

Additionally, could you let us know if there is a deadline for your final project and if this PR needs to be merged before then?

Thank you :)

Fre0Grella commented 3 months ago

Hi @Schefflera-Arboricola, the PR is ready for review although i aim to improve the performance further. The dead line of the project is due the 18 of September. I doesn't need the PR to be merged before that

Schefflera-Arboricola commented 2 months ago

Thanks @Fre0Grella! I think instead of nxp.cpu_count using nxp.get_n_jobs would have fixed the failing CI tests because we renamed it recently. If you still have the bandwidth please feel free to merge the main branch again. Really sorry for the delayed review!

Fre0Grella commented 2 months ago

Thanks @Fre0Grella! I think instead of nxp.cpu_count using nxp.get_n_jobs would have fixed the failing CI tests because we renamed it recently. If you still have the bandwidth please feel free to merge the main branch again. Really sorry for the delayed review!

Not sure if i'm doing something wrong but replacing nxp.cpu_count to nxp.get_n_jobs isn't working. On the test i get AttributeError: module 'nx_parallel' has no attribute 'get_n_jobs'

Schefflera-Arboricola commented 2 months ago

All the tests are passing now and the style test should also pass once you update this PR branch with the recent fix that got merged. Thank you :)

Fre0Grella commented 2 months ago

It would be awesome if we can merge this PR before 1st of october so i can present the project directly from the NetworkX repository to the exam commision. Thanks a lot for the support along the project. I'm also interest in further contribuition to the NetworkX project

Fre0Grella commented 1 month ago

@Schefflera-Arboricola hey, the PR is ready to be merged, it only need a Review

dPys commented 1 month ago

@Fre0Grella -- I should mention (since I haven't yet) that this is excellent work. @Schefflera-Arboricola-- hopefully after the remaining polishing we can get this thing merged?