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

Fix GraphMatch random_state behavior with n_init > 1, parallelize #770

Merged
merged 12 commits into from Apr 16, 2021

Conversation

asaadeldin11
Copy link
Contributor

@asaadeldin11 asaadeldin11 commented Apr 16, 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

Closes #761
Closes #765.

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

Fixes bug where random_state when n_init > 1 was always the same. Also parallelizes over inits

@netlify
Copy link

netlify bot commented Apr 16, 2021

Deploy preview for graspologic ready!

Built with commit 6c5c27c

https://deploy-preview-770--graspologic.netlify.app

graspologic/match/gmp.py Outdated Show resolved Hide resolved
@bdpedigo
Copy link
Collaborator

added a test, feel free to change it

@bdpedigo bdpedigo self-requested a review April 16, 2021 17:00
Copy link
Collaborator

@bdpedigo bdpedigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this is good to go, @asaadeldin11 you can decide whether to just leave a comment about efficiency or see if the n_jobs = 1 fix helps performance, otherwise I'm ready to merge

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

@asaadeldin11 somehow that broke old tests, any idea why? maybe the tests just werent that stable to initialization? Or do we think something else is going on?

@asaadeldin11
Copy link
Contributor Author

asaadeldin11 commented Apr 16, 2021

@asaadeldin11 somehow that broke old tests, any idea why? maybe the tests just werent that stable to initialization? Or do we think something else is going on?

Strange, they were passing for me locally. The padded sgm test makes sense, i remove two nodes and get 48/50 match ratio, so I'll change the 1.0 to 0.95 since there's no reason to expect the isolates to match correctly.
The second one I'm less sure about, but I think it might be resolved by changed n in test_sim from 90 to something larger (120 or 150). Thoughts?

@bdpedigo
Copy link
Collaborator

@asaadeldin11 somehow that broke old tests, any idea why? maybe the tests just werent that stable to initialization? Or do we think something else is going on?

Strange, they were passing for me locally. The padded sgm test makes sense, i remove two nodes and get 48/50 match ratio, so I'll change the 1.0 to 0.95 since there's no reason to expect the isolates to match correctly.
The second one I'm less sure about, but I think it might be resolved by changed n in test_sim from 90 to something larger (120 or 150). Thoughts?

is there any other way to just make the problem easier?

@bdpedigo bdpedigo merged commit e665f8c into dev Apr 16, 2021
PRs automation moved this from Ready For Merge to Done Apr 16, 2021
@bdpedigo bdpedigo deleted the gmp_rand branch April 16, 2021 20:21
@asaadeldin11
Copy link
Contributor Author

asaadeldin11 commented Apr 16, 2021

is there any other way to just make the problem easier?

I can just make the similarity matrix the identity matrix, and make it an ER graph?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PRs
Done
2 participants