-
Notifications
You must be signed in to change notification settings - Fork 33
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 function for Kuramoto model synchronization #159
Conversation
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 your contribution @saad1282 ! I left a few minor comments that hopefully won't be too time-consuming to address.
The algorithm it self looks good to me. It's clean and easy to read. However I'm a bit concerned about performance. I assume the operations inside the main loop are hard to vectorize since they iterate over the links/triangles, is that correct? Or are you aware of some way of writing down these operations using matrices? If we can get rid of those inner loops and write the entire computation in a single/few matrix computations then we'd gain a lot in performance.
H1 = xgi.random_hypergraph(100, [0.05, 0.001], seed=0) | ||
r = xgi.compute_kuramoto_order_parameter(H1, 1, 1, np.ones(100), 10, 0.002) | ||
|
||
assert len(r) == 10 |
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.
Question: this only tests the length of the output, which should always match the fifth argument in the call in the previous line, no? If so, I suggest hard-coding the actual numeric output and testing against that. There should be no problem since we are fixing the seed two lines above anyway.
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.
We totally agree that there should be more rigorous tests. We will have to figure out better tests and your suggestion is a good option.
Hi @leotrs. @saad1282 and I worked on implementing a numpy version (added below) and I haven't tested line-by-line but I suspect that forming the adjacency matrix and tensor is relatively expensive. Suggestions welcome!
|
links = H_int.edges.filterby("size", 2).members()
A = np.zeros([n, n])
for i, j in links:
A[i, j] += 1
A[j, i] += 1
triangles = H_int.edges.filterby("size", 3).members()
B = np.zeros([n, n, n])
for t in triangles:
for i, j, k in product(t, repeat=3):
B[i, j, k] += 1 I'm p sure that with some advanced slicing you could set these entries of r2 = np.zeros(n, dtype=complex)
r1 = A.dot(np.exp(1j*theta))
r2 = np.exp(-1j*theta).dot(B).dot(np.exp(2j*theta)) There's no need for the double definition of |
|
Added a dynamics module and created a Kuramoto model synchronization function.