New graph generator for the Kneser graph #7146
Conversation
There was a problem hiding this comment.
Thanks @Transurgeon
Just a quick note on general policies for future reference - you should avoid referring to GPL-licensed code when developing your own implementations, as "adapting" implementations (even if they are in a different programming language) arguably falls under the terms of copying/modification in the GPL license. The better thing to do (IMO) is to refer to the primary sources and implement from there. Not a huge deal in this case as the algorithm is simple and derivable from linked sources. I just wanted to point this out so that it was documented: for future proposals, make sure to avoid referring explicitly to GPL-licensed code when developing your own implementation!
|
@rossbar Thanks for pointing this out and sorry about that. |
|
Unfortunately, it is best to read pseudo-code and algorithm descriptions and not to look at the GPL code if you suspect you will want to write a non-GPL version of the algorithm. It is very hard to make sure that code you write is not based on GPL code if you have read that code. That said, if you can clear your mind and start from scratch and build your code only using other sources (and not memories of the GPL code). Then you can probably get that to work in a courtroom. Perhaps someone else has a better description. But I think the perspective of the people behind GPL is that all the code related to GPL is GPL code. And we shouldn't be writing non-GPL code after having read the GPL code. It's a pain, and restricts code reuse. But that is what copyrights are all about. We need to give proper power over usage to the authors of the code. |
|
Let's move forward on this. The code is pretty short. The Kneser graph is of interest and quite understandable. I have another question : you move some other functions (or it looks like that in the diff). Did those function change? Or did they get moved? What is your intent with them? (If it is to alphabetize them then maybe we should back those changes out to reduce churn in the blame. finding a function definition within a file should be easy without the alphabetizing (maybe?) |
There was a problem hiding this comment.
Let's leave the functions in the order they were so the "blame" doesn't think that we changed those functions in this PR.
Can you add this new function to the docs with an entry in doc/reference/generators.rst in the "classic" section?
I have some picky questions (see below).
I didn't change those functions, it was only to keep the alphabetical order (I was a bit picky seeing a difference between the function definitions and the list in the |
|
Hey @dschult thanks for looking into this, I have made the changes you requested. P.S. Just wanted to remind you about my other PR #6928 in case you forgot. I thought it would be really awesome if we could get it merged, since we made some progress on the notation and I hope I helped you understand the algorithm. |
There was a problem hiding this comment.
Thanks for that!
Can you add blank lines between the doc_string sections for the function:
- after the first line
- before the Parameters
- before Returns
- before Examples
Also, we need to hook the doc_string to the sphinx webpage generator by adding this function to doc/reference/generators.rst in the classic section.
Thirdly, can you add @nx._dispatchable(graphs=None) to the line before the function definition. That's our decorator that allows "backends" to provide features like parallel computation, etc.
|
Hi Dan, thanks for your feedback. I managed to generate the docs by adding the function in the hook for sphinx. (Looks good to me!). What should we do about the conflicts with the |
There was a problem hiding this comment.
This is ready as far as I can tell.
I approve this PR.
The conflicts were related to the change from @dispatch to @dispatchable so the changes you made fixed the conflicts. (conflicts are just where git can't figure out whether to accept changes from the PR or changes done on main since the PR was branched from main.)
Thanks for this--
There was a problem hiding this comment.
Generally LGTM, thanks @Transurgeon - just one final comment about the description of the n parameter. It'd be great to take a crack at improving that, but if it's not possible then it's not a blocker!
I also took the liberty of pushing up a few minor docstring formatting changes, so make sure to pull before making any additional changes!
Adds the kneser_graph graph generation function. Co-authored-by: Transurgeon <peter.zijie@gmail.com> Co-authored-by: Ross Barnowski <rossbar@caltech.edu>
This PR implements a new graph generator, namely the kneser graph with parameters
nandk.The implementation is adapted from the same generator in sageMath, see here for the referenced source code.
I also used this resource from Wolfram to understand some properties of the kneser graphs.
I have added some corresponding tests and I also reformatted some generators so that they would all be in alphabetic order.
P.S I also wasn't sure if I should include the
create_usingparameter and also thenx.dispatchwhich I think comes from the graphBLAS backend? Please let me know if any of these two additions would be necessary and I can make the changes accordingly.