graspologic icon indicating copy to clipboard operation
graspologic copied to clipboard
verified publisher icon graspologic-org

Add sparse support to multigraph LCC functions

Open bdpedigo opened this issue 4 years ago • 7 comments

  • [ ] Does this PR add any new dependencies?
  • [x] Does this PR modify any existing APIs?
    • [x] Is the change to the API backwards compatible?
  • [ ] Have you built the documentation (reference and/or tutorial) and verified the generated documentation is appropriate?

Reference Issues/PRs

What does this implement/fix? Briefly explain your changes.

Any other comments?

bdpedigo avatar Jun 23 '21 20:06 bdpedigo

❌ Deploy Preview for graspologic failed.

🔨 Explore the source changes: ab14d8bbd944a86f5cb2558708d71f4e768465d4

🔍 Inspect the deploy log: https://app.netlify.com/sites/graspologic/deploys/612518dd0f7f6a000820c040

netlify[bot] avatar Jun 23 '21 20:06 netlify[bot]

@nyecarr you mentioned sparse omni the other day, I think fixing this will be necessary as part of that workflow

bdpedigo avatar Jun 30 '21 23:06 bdpedigo

looks like theres some formatting issues but the rest looks fine to me.

asaadeldin11 avatar Aug 23 '21 20:08 asaadeldin11

formatting issues are probably because of isort

daxpryce avatar Aug 23 '21 21:08 daxpryce

@daxpryce it looks like the functions multigraph_lcc_intersection and multigraph_lcc_union claim they support lists of networkx graphs (e.g. here: https://github.com/microsoft/graspologic/blob/e5d896d3798aca526d592d4925c7f8be27e7b622/graspologic/utils/utils.py#L643) but in reality, they don't (e.g. here: https://github.com/microsoft/graspologic/blob/e5d896d3798aca526d592d4925c7f8be27e7b622/graspologic/utils/utils.py#L670)

I was in the process of adding sparse support in this PR when I came across this - I wonder whether you all have any use for these things working on networkx or not? If not, we could just take out of our contract with the user for now?

bdpedigo avatar Dec 13 '21 19:12 bdpedigo

note that I think y'all care about this functionality (which I believe you reimplemented here https://github.com/microsoft/graspologic/blob/e5d896d3798aca526d592d4925c7f8be27e7b622/graspologic/pipeline/embed/omnibus_embedding.py#L162), not that I am necessarily advocating for these util functions to support networkx

bdpedigo avatar Dec 13 '21 19:12 bdpedigo

decision from talk on 12/14: just make this accept AdjacencyMatrix or whatever, no nx for now

bdpedigo avatar Dec 14 '21 18:12 bdpedigo