-
Notifications
You must be signed in to change notification settings - Fork 3
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
RWR Implementation #12
base: master
Are you sure you want to change the base?
Conversation
src/PathLinker subrepo: subdir: "src/PathLinker" merged: "d4a44c9" upstream: origin: "https://github.com/Murali-group/PathLinker.git" branch: "master" commit: "d4a44c9" git-subrepo: version: "0.4.3" origin: "???" commit: "???"
Thanks @blessyantony9! I see you copied the PageRank.py to src/FastSinkSource/src/algorithms, and also added the PathLinker repo to src. It looks like the PathLinker repo isn't used anywhere, is that right? Can we remove it? |
Thank you @jlaw9 for reviewing. Yes, the aim is to reuse PageRank from within PathLinker which is added as a sub-repo in this repo. However, unfortunately, there is version incompatibility for one of the packages between PathLinker and SARS-CoV-2-network-analysis: networkX. In PathLinker it is v1.11 whereas in the current repo it is v2+. So, until we upgrade the version of this package in the whole of PathLinker, I have copied the code of PageRank over to src/FastSinkSource/src/algorithms and updated the usages only in this file as a tactical solution. I have included this as a TODO comment in the file. |
Ah ok I see. I appreciate the work you did to incorporate the networkx graphs to be able to use that PageRank implementation. Since we already have the other algorithms implemented in terms of matrix-vector multiplication, I think it would keep the code simpler (and faster than networkx) to do the same for RWR i.e., I had forgotten that I had added such an implementation to the annotation_prediction repo, which is the updated version of the FastSinkSource repo (I had planned to update the FastSinkSource part of this repo to use that annotation_prediction repo, but hadn't gotten around to it unfortunately). It looks like there are a couple of features that PageRank.py code has that my rwr.py doesn't have, such as the ability to use non-uniform weights for the restart probabilities, but I think that would be easy enough to add if we we wanted to use that. |
Thank you @jlaw9 for the inputs about using matrix multiplication. I was unaware of the existing matrix implementation. I too implemented RWR using sparse matrices for another project within the N3C Enclave (Palantir) which too assumes default weight=1, but as you mentioned can be tweaked to incorporate non-uniform weights. I'll discuss the available matrix implementations with Dr.Murali and update this pull request accordingly (if we decide to go with the matrix version). |
@blessyantony9 please go ahead and use the matrix implementation from the other project here as well. |
Thank you @tmmurali. I'll update the code to use RWR from the annotation_prediction repo. |
@jlaw9 As discussed, I've added the PageRank Matrix implementation. Apologies for the delay. Please let me know if you have any questions or if any changes are required. |
Added RWR implementation using PageRank from PathLinker.