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
Implementation of $S^1$ model #6858
Conversation
Hi! Can I ask for the code review? @MridulS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR @robertjankowski! It looks good!
I have made some comments about code/API organization in the review. Let me know what do you think.
Also I think we can speed up a bunch of calculations with numpy vectorisation but it's not a major blocker here. It's fine to keep this graph generator with vanilla python :)
* Unpin scipy upperbound for tests * Avoid scipy 1.11.0 and 1.11.1
…tworkx#6856) MAINT: Enforce pre-2.0-style reprs for numpy scalars. This will silence doctest errors related to changes in the NumPy scalar reprs per NEP 51. The goal is to aid in transitioning to new numpy scalars with the minimum possible churn in code. In general, the transition plan would be something like: - Leave this PR in place to suppress doctest failures from changes in numpy scalar reprs. This way, all doctests can still be run. - Update / modify code and/or tests as needed to limit/replace the use of numpy scalars where appropriate. - When NumPy 2.0 becomes the minimum supported version for NetworkX, These 5 lines can be removed and any remaining issues related to scalar representations fixed (hopefully there aren't many left... see previous bullet).
* Unpin numpy nightly wheels * Add note so we don't forget to cleanup from networkx#6856
)" (networkx#6859) * Revert "Pin sphinx<7 as temporary fix for doc CI failures (networkx#6680)" This reverts commit a068e47. * Bump minimum dependencies
Add benchmarking suite to the main repo based on asv. Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
…rkx#6867) add docs for dfs_predecessor and dfs_successor arg source
…#6866) Fix Issue 6865 with round-off causing nans via sqrt
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
* Change `_dispatch` to a class instead of a closure * pickle pickle! * Support `can_run(name, args, kwargs)` function for backends - Remove `is_testing=` keyword in `convert_from_nx` (for now) - Delay computing `__signature__` of dispatched functions - Fix `name` -> `self.name` in f-string * Move dispatch type checking into constructor * Slightly faster import time * Fast path when dispatching if no backends are installed This requires "nx-loopback" to only be available when it's being tested. * Always include nx-loopback plugin when testing * Does this work? (trying to fix tests on Python <3.10) * This should work * better * Add `NETWORKX_AUTOMATIC_BACKENDS` to enable automatic conversions to backends `NETWORKX_AUTOMATIC_BACKENDS` is a comma-separated environment variable, so multiple backends can be specified in priority order. Rename `NETWORKX_TEST_FALLBACK_TO_NX` to `NETWORKX_FALLBACK_TO_NX`, since the latter can be used with `NETWORKX_AUTOMATIC_BACKENDS` environment variable. Refactor methods on `_dispatch` class: - add `_can_backend_run` helper function - should we expose this to users? - add `_convert_arguments` to convert `*args` and `**kwargs` - should we expose this to users? If so, should we use `*args` and `**kwargs` in the signature? - remove `is_testing=` keyword argument from `_convert_and_call` - add `_convert_and_call_for_tests` to use when testing Also, add a few `# Future work:` code comments to be more clear about development progress. * haha, oops * Automagically add `backend=` keyword argument to dispatched functions * Clean up exception raising * Also catch `NotImplementedError` from converting graphs when dispatching
…ctorized methods. (networkx#6879) * modified: networkx/drawing/layout.py - Replaced for-loops in :function:`rescale_layout` with numpy vectorized methods. - No new tests as output should not have changed and no new features were introduced. - Tested with :method:`TestLayout.test_rescale_layout_dict` in drawing/tests/test_layout.py. * Update networkx/drawing/layout.py Setting `lim` to the max absolute val of `pos` per suggestion. Co-authored-by: Ross Barnowski <rossbar@berkeley.edu> --------- Co-authored-by: Ian Thompson <ian.thompson@hrblock.com> Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
Revert "Clarify that basis generates simple cycles only (networkx#6882)" This reverts commit 7febe96.
* Accept proposed formatting changes * Accept proposed formatting changes * Update fast label propagation algorithm (type checks, variable names, docs) --------- Co-authored-by: Lovro Šubelj <lovro.subelj@fri.uni-lj.si>
* updating TSP example docs * adding hyperlink * trying another way for hyperlink * trying new path * trying new path again * trying to use func instead * removing hyperlink as its too complicated * restoring changes to the rest of the paragraph * fix sphinx internal ref link --------- Co-authored-by: Transurgeon <peter.zijie@gmail.com> Co-authored-by: Mridul Seth <git@mriduls.com>
* ENH: let users set a default value in get_attr methods Co-authored-by: Dan Schult <dschult@colgate.edu>
Thank you very much for the review @MridulS. Let me know if some more changes are needed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh... thanks for the connection to H2 as well as S1. It gives be a better idea of what is crucial to the arrangement. It isn't specific to S1, or to H2. There are many other spaces that this geometry could be embedded inside. The crux is that the distance measure is periodic. How about the name: soft_random_periodic_geometric_graph
.
That is getting a little long, so maybe soft_periodic_geometric_graph
? But sometimes brevity causes confusion.
Thank you for your comments! I checked with the literature, and this kind of model is called the geometric soft configuration model. Hence, I propose to use it as a function name, i.e., I made |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice Name! Thanks for coming up with that. :}
This review is getting into the weeds a little, but I think that means we are getting near the end. :)
It strikes me that kappa is always created by multiplying by kappa_0 and then used by dividing by kappa_0. Could we just scale out the kappa_0? Also the kappa_0 used when creating it is from a formula, while when dividing by it it seems to be the minimum value of all kappas. So, I might be getting confused by all that. But it seems that we could scale all the kappas by kappa_0 at some poin and reduce a lot of computation.
I was surprised to find that (kappa_c / kappa_0) ** (1 - gamma)
is just 1 / n
. Is this really right? or did I do some algebra mistake... That simplifies formulas quite a bit. I'm used to published formulas already being simplified. Are these from the publication?
It'd be great if we had test(s) which tested values. We've got a few checking e.g. that increasing beta decreases average clustering, and that beta<1 gives positive average clustering. Are there others? Perhaps with average degree while changing either the kappas or mean_degree?
Thanks @dschult for the review! I think now it looks better. Regarding the kappas you're right and we can simplify it. The equation I used allowed us to change the kappa cutoff (kappa_c), but in our case, it is defined using kappa_0. I added a few more tests to check the connectivness and mean degree of the generated networks. I hope we are close to finishing this PR :) |
Hi @dschult, do you have any further comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two other nit-picky algebra simplifying comments.
We really are close... :)
Thanks @dschult! I think this will be the last round :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Thanks @robertjankowski !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor tweaks -- spelling and add dispatch decorator support.
Co-authored-by: Dan Schult <dschult@colgate.edu>
Co-authored-by: Dan Schult <dschult@colgate.edu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, thanks @robertjankowski
I left one minor comment inline, but my only real comment is that I'm not crazy about the overloaded signature - i.e. the fact that you use different incantations of the same function (either with n
, gamma
, and mean_degree
or by directly providing the degree of each node with kappas
). OTOH I can't really think of a better way to do it. The only idea that comes to mind is to create two functions, one named geometric_soft_configuration_graph
which accepts kappa
as input, and a second geometric_soft_configuration_from_powerlaw
(e.g.) that accepts n, gamma, and mean_degree. YMMV on whether that's really any different/better (seems pretty subjective to me).
Another option would be to just drop the kappas
route entirely. In your experience, is it common to have hidden-degree distributions known a priori? If this is a "someone might in principle want this" feature, then I'd be inclined to drop it for now and add it at the point where there's demand. If, however, this is something that comes up all the time in applications then dropping it would not make sense. I'm no expert so I defer to your opinion!
networkx/generators/geometric.py
Outdated
|
||
# Map hidden degrees into the radial coordiantes | ||
zeta = 1 if beta > 1 else 1 / beta | ||
kappa_min = min(list(kappas.values())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, but I don't think the list is necessary here - min
should work over dict views.
kappa_min = min(list(kappas.values())) | |
kappa_min = min(kappas.values()) |
Hi @rossbar! Thanks for your comment! I would be more inclined to keep the definition of the function as it is, even though it might not be super clear for everyone. The point I wanted here to make is that this model is general enough to take any sequence of hidden degrees, and powerlaw distribution is just a particular (however often used) case. Splitting it into two functions might confuse some people since the logic would be the same in both functions. From my experience, it is useful to generate the graph from the predefined hidden degrees. For instance, you can first infer the kappas from the real network and use them as the input here to generate similar synthetic networks to your network. |
Sounds reasonable for me, thanks for taking the time to put together the summary! |
Add a geometric_soft_configuration_graph graph generator function implementing the S1 model. --------- Co-authored-by: Ross Barnowski <rossbar@berkeley.edu> Co-authored-by: Mridul Seth <seth.mridul@gmail.com> Co-authored-by: Dan Schult <dschult@colgate.edu>
Add a geometric_soft_configuration_graph graph generator function implementing the S1 model. --------- Co-authored-by: Ross Barnowski <rossbar@berkeley.edu> Co-authored-by: Mridul Seth <seth.mridul@gmail.com> Co-authored-by: Dan Schult <dschult@colgate.edu>
This is an implementation of$\mathbb{S}^1$ model by [1]. It is a geometric soft configuration model which produces synthetic networks with similar properties as in real networks, i.e., high level of clustering, heterogeneous degree distribution, or small-world property.
[1] Serrano, M. Angeles, Dmitri, Krioukov, and Marián, Boguñá. "Self-Similarity of Complex Networks and Hidden Metric Spaces". Phys. Rev. Lett. 100 (2008): 078701.