Padded SGM #510
Padded SGM #510
Conversation
* add padded sgm to gmp.py * resolve some docs issues * add tests * remove excessive gmp tests for performance * doc changes * add tutorial, remove math from docs * Update tutorial.rst * Update gmp.py * Update padded_sgm.ipynb Co-authored-by: Benjamin Pedigo <benjamindpedigo@gmail.com>
|
Deploy preview for graspy ready! Built with commit 3fa29fe |
There was a problem hiding this comment.
@asaadeldin11 can't tell if this is ready for review since it looks like it was requested automatically, can you lmk when all the build checks are working? Then im guessing we can get this in really quickly
Yup, not quite ready for review. I'll let you know once I've addressed your previous feedback and have checks working. |
There was a problem hiding this comment.
Notebook comments:
- I still find the use of "core vertices" unclear, for example in "Match ratio of core vertices with naive padding: 1.0
" could you just say "Match ratio of nodes left in G1" or something more eloquent but to that effect? - looks like something got messed up with the notebook as both plots look the same in netlify render.
- please note somewhere that the example was adapted from a paper (that's true right?) so we are giving proper attribution
- super nitpicky but could you change name of files (tutorial and anything else I missed) to
padded_gmsince seeds are not required and that matches the name of the notebook.
Aside from those minor comments I am happy to merge!
|
@Nyecarr @bryantower @dwaynepryce if anyone can have a look I think this PR is ready for review on MSFT side |
|
Github is being weird so i can't re-request a review for myself, but I officially approve. |
|
@dwaynepryce any idea why the microsoft checks havent finished running on this? also, jovo was hoping to send this code to someone - do you think someone on the MSR side would have time to give this a quick look this week? |
|
This looks fine to me @asaadeldin11, but I did have a question about why |
|
@dwaynepryce thanks! did you see my notes about the status checks? That is preventing merge |
|
According to the pipeline it never got a request to build. I found an older one for this PR and told it to run again, so hopefully this works for us? Edit: Oh good, it invalidated the rest of the build pipeline. 😭 |
|
I have no idea what caused it, but deleting the build pipeline in Azure DevOps and recreating it seems to have made it work again. Sigh. |
Reference Issues/PRs
Fixes #316, supersedes #472.
What does this implement/fix? Explain your changes.
This PR brings padded SGM to graspologic, allowing users to pass in input matrices of different sizes to
GraphMatch. This is an implementation of the padding scheme describing in Seeded Graph Matching (section 2.5). Both schemes described (adopted and naive), are implemented here, allowing users the option to choose which they'd prefer to use.This PR also removes some redundant tests in the match submodule.
Any other comments?
Proof of effectiveness can be found in the following notebook, where Figure 3 from SGM is replicated.