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 single node with self loop check to local and global reaching centrality #7350

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mohamedrezk122
Copy link
Contributor

This PR partially solves #6914

I have just added a check for single node with self loops, this behavior is meaningless and should raise NetworkXPointlessConcept.
Also, I have added the corresponding test cases.

@dschult dschult changed the title Add single node with self loop check to local and global reaching centraliy Add single node with self loop check to local and global reaching centrality Mar 27, 2024
@rossbar
Copy link
Contributor

rossbar commented Mar 27, 2024

@mohamedrezk122 if you have the bandwidth it'd be good to push this one forward as I think there's a clear path to getting this fix in!

@mohamedrezk122
Copy link
Contributor Author

mohamedrezk122 commented Mar 27, 2024

@rossbar
I have pushed the changes, regarding the global_reaching_centrality not to handle the raising, the function still return the division by zero error as the handling of the error in local_reaching_centrality happens if paths are None; however, the global_reaching_centrality calls the local reaching with paths computed

so there are two options here

  • move the check upward in local reaching and we don't want global to handle the raising, but other raises would be missed up ig.
  • make both handle the raising

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the iteration @mohamedrezk122 . I took the liberty of pushing up a different organization of conditionals to better illustrate what I had been thinking. It's true that the self-loop check was originally in the paths=None codepath; however, with just a little bit of re-organization, I think that can be pulled out (as in 193ef60) to de-duplicate the check.

There is of course a tradeoff in performance here: organizing it in this way essentially means that the total_weight computation happens twice for global_reaching_centrality: once in the grc call and once in the lrc call. However; I believe the tradeoff is worth it in order to guarantee that the exception always comes from local_reaching_centrality. The reason being that it's really the local_reaching_centrality for which the single-node-selfloop case is undefined; global_reaching_centrality (AFAICT) is just the ratio of the lrc to the maximum lrc across all nodes. Note also that the total_weight = G.size(weight=weight) is very unlikely to ever be the bottleneck in these computations - the computation of shortest paths is likely always to be much more expensive!

I also updated the tests to catch this fact: note that the match= statement in the pytest.raises explicitly captures that the exception originates from local_reaching_centrality in both cases!

Anyways - LMK what you think. This is the most elegant way I could think to solve the issue with the least code duplication and highest precision, but there are certainly other ways to do it!

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will go ahead and approve this, but it should certainly have another set of eyes to make sure the things that I pushed up are reviewed as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants