Skip to content
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

Add sparse support to multigraph LCC functions #804

Closed
wants to merge 13 commits into from

Conversation

bdpedigo
Copy link
Collaborator

@bdpedigo bdpedigo commented Jun 23, 2021

  • Does this PR add any new dependencies?
  • Does this PR modify any existing APIs?
    • 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?

@netlify
Copy link

netlify bot commented Jun 23, 2021

❌ Deploy Preview for graspologic failed.

🔨 Explore the source changes: ab14d8b

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

@bdpedigo bdpedigo marked this pull request as ready for review June 30, 2021 23:52
@bdpedigo bdpedigo requested a review from nicaurvi June 30, 2021 23:53
@bdpedigo
Copy link
Collaborator Author

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

@asaadeldin11
Copy link
Contributor

asaadeldin11 commented Aug 23, 2021

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

@daxpryce
Copy link
Contributor

formatting issues are probably because of isort

@bdpedigo bdpedigo added this to Needs Triage in PRs via automation Aug 24, 2021
@bdpedigo bdpedigo moved this from Needs Triage to Ready For Merge in PRs Aug 24, 2021
@bdpedigo
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@bdpedigo bdpedigo added this to Needs review in bdpedigo Jan 28, 2022
@bdpedigo bdpedigo closed this May 10, 2024
PRs automation moved this from Ready For Merge to Done May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PRs
Done
bdpedigo
Needs review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants