Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

fix possible NaN in closeness centrality. Closes #1555 #1568

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sbromberger
Copy link
Owner

No description provided.

@codecov
Copy link

codecov bot commented Jun 4, 2021

Codecov Report

Merging #1568 (f05cdea) into master (1769686) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f05cdea differs from pull request most recent head e2b5f8b. Consider uploading reports for the commit e2b5f8b to get more accurate results

@@           Coverage Diff           @@
##           master    #1568   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files         106      106           
  Lines        5551     5553    +2     
=======================================
+ Hits         5520     5522    +2     
  Misses         31       31           

@sbromberger
Copy link
Owner Author

cc @jpfairbanks or @simonschoelly for a quick review please. Thanks.

@jpfairbanks
Copy link
Contributor

What about the case where sigma is negative because of integer arithmetic wrap around? Do you still want 0 closeness?

@sbromberger
Copy link
Owner Author

Good point. I wonder if it makes sense to check against 0.0 and the again against < 0.0 and throw an IntegerOverflowError or something.

@jpfairbanks
Copy link
Contributor

An extra branch that won't get hit except in exceptional circumstances seems worth it to me. Well, can we prove that this can't overflow because it is a sum of distances and the distances are bounded by nv(g)? I guess for Int16 overflow is a real possibility if you have a path graph?

@sbromberger
Copy link
Owner Author

Actually, I don't see how we even get to σ == 0. We explicitly check to see whether the degree is zero in line 13, and by definition since we're using Dijkstra any edge must have a positive nonzero weight. What's really going on here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants