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

Add tensor centralities #600

Merged
merged 27 commits into from
Nov 12, 2024
Merged

Add tensor centralities #600

merged 27 commits into from
Nov 12, 2024

Conversation

nwlandry
Copy link
Collaborator

@nwlandry nwlandry commented Oct 17, 2024

Added tensor methods to the centrality module. Fixes #505.

This PR also fixes Issue #616, where the node_edge_centrality was not converging correctly.

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 99.06977% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.42%. Comparing base (1fa9839) to head (0e5d138).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
xgi/algorithms/centrality.py 98.88% 1 Missing ⚠️
xgi/utils/tensor.py 99.16% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #600      +/-   ##
==========================================
+ Coverage   93.21%   93.42%   +0.20%     
==========================================
  Files          62       63       +1     
  Lines        4733     4912     +179     
==========================================
+ Hits         4412     4589     +177     
- Misses        321      323       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nwlandry
Copy link
Collaborator Author

@ilyaamburg --- I rewrote the pairwise_incidence method and I have a few questions on this: (1) does order matter when adding (i, j) to the dictionary of pairs? (2) there was an "r" parameter for this function, but because r seemed to just be the maximum edge size, the loop where you added the self-loops (node, node) seems to always be true. So I removed r as an argument, and removed the if-statement. Let me know your thoughts!

xgi/utils/tensor.py Outdated Show resolved Hide resolved
xgi/utils/tensor.py Outdated Show resolved Hide resolved
xgi/utils/tensor.py Outdated Show resolved Hide resolved
xgi/utils/tensor.py Outdated Show resolved Hide resolved
xgi/utils/tensor.py Outdated Show resolved Hide resolved
xgi/utils/tensor.py Outdated Show resolved Hide resolved
@maximelucas
Copy link
Collaborator

Thanks Nich. A few comments mainly about documentation. Let's see what @ilyaamburg has to say about your question too.

@ilyaamburg
Copy link

ilyaamburg commented Nov 1, 2024

@ilyaamburg --- I rewrote the pairwise_incidence method and I have a few questions on this: (1) does order matter when adding (i, j) to the dictionary of pairs? (2) there was an "r" parameter for this function, but because r seemed to just be the maximum edge size, the loop where you added the self-loops (node, node) seems to always be true. So I removed r as an argument, and removed the if-statement. Let me know your thoughts!

Hey Nick, I am liking the new pairwise_incidence that uses combinations and defaultdict with set values, which should be fine, since the order does not matter as long as you end up taking one of each pair, and the order of the values does not matter either. The r and corresponding if statement should stay, though, because we only want self loops in the case l (= |e|) < r; that is, we do not want to add them if the hyperedge size is maximal.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@nwlandry nwlandry linked an issue Nov 6, 2024 that may be closed by this pull request
@nwlandry
Copy link
Collaborator Author

nwlandry commented Nov 6, 2024

@ilyaamburg --- I think I addressed all of your comments if you want to take a final look. From looking at the test coverage, it seems like the _get_gen_coef_fft_fast_array function is never called with the test hypergraphs that I used. Is there a specific hypergraph that I could use to test this?

Also, could you provide docstrings in this comment thread briefly describing the two helper functions, _get_gen_coef_subset_expansion and _get_gen_coef_fft_fast_array?

We are almost there!!

@ilyaamburg
Copy link

@ilyaamburg --- I think I addressed all of your comments if you want to take a final look. From looking at the test coverage, it seems like the _get_gen_coef_fft_fast_array function is never called with the test hypergraphs that I used. Is there a specific hypergraph that I could use to test this?

Also, could you provide docstrings in this comment thread briefly describing the two helper functions, _get_gen_coef_subset_expansion and _get_gen_coef_fft_fast_array?

We are almost there!!

@nwlandry thanks so much for this! Hm, _get_gen_coef_fft_fast_array is really the workhorse that makes everything scale so I'm shocked it's not being called. It should get called whenever the hyperedge size is in the purple range:
Screenshot 2024-11-12 at 10 15 04 AM
As for the docstrings, here is a draft:
def _get_gen_coef_subset_expansion(edge_values, node_value, r):
"""Computes the generating funciton coefficient of order r using subset expansion.

Parameters
----------
edge_values : NumPy array
    Array of values from the a vector corresponding to
    nodes in the hyperedge.
node_value : float
    The value in a corresponding to the node being processed.
r : int
    Desired order to get coefficient for.

Returns
-------
float
    Generating function coefficient of order r.

See Also
--------
_get_gen_coef_fft_fast_array

References
----------
Scalable Tensor Methods for Nonuniform Hypergraphs,
Sinan Aksoy, Ilya Amburg, Stephen Young,
https://doi.org/10.1137/23M1584472
"""

def _get_gen_coef_fft_fast_array(edge_without_node, a, node, l, r):
"""Computes the generating funciton coefficient of order r using the Fast Fourier Transform.

Parameters
----------
edge_without_node : list
    Array of node indices corresponding to
    all nodes in the hyperedge but the one being processed.
a : NumPy array
    The vector to multiply the tensor by.
node : int
    The index of the node being processed.
l : int
    Number of nodes in the hyperedge.
r : int
    Desired order to get coefficient for.

Returns
-------
float
    Generating function coefficient of order r.

See Also
--------
_get_gen_coef_subset_expansion

References
----------
Scalable Tensor Methods for Nonuniform Hypergraphs,
Sinan Aksoy, Ilya Amburg, Stephen Young,
https://doi.org/10.1137/23M1584472
"""

@nwlandry
Copy link
Collaborator Author

Everything is now addressed, and so I will now merge this once tests pass.

@nwlandry nwlandry merged commit 1bdba44 into main Nov 12, 2024
24 checks passed
@nwlandry nwlandry deleted the add-tensor-centralities branch November 12, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with node_edge_centrality Add tensor functionality to eigenvector centralities
3 participants