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

ENH: include assign_self_weight (formerly fill_diagonal) in Graph #670

Merged
merged 10 commits into from
Dec 29, 2023

Conversation

martinfleis
Copy link
Member

Closes #668

@knaaptime this includes pandas-fu version as well as roundtrip via sparse for now. Timing below.

import geopandas as gpd
from geodatasets import get_path
from libpysal import graph

df = gpd.read_file(get_path("geoda.ncovr"))
contig  = graph.Graph.build_contiguity(df)

%%timeit
contig.fill_diagonal()
5.31 ms ± 108 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

%%timeit
contig.fill_diagonal_sparse()
16.3 ms ± 92.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Here, the sparse version is about 3x slower. This ratio seems to be more or less consistent with any size of the graph.

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (27c1f49) 85.0% compared to head (0fe196f) 85.1%.
Report is 8 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #670   +/-   ##
=====================================
  Coverage   85.0%   85.1%           
=====================================
  Files        139     139           
  Lines      14889   14903   +14     
=====================================
+ Hits       12663   12677   +14     
  Misses      2226    2226           
Files Coverage Δ
libpysal/graph/base.py 97.7% <100.0%> (+<0.1%) ⬆️
libpysal/graph/tests/test_base.py 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@martinfleis martinfleis marked this pull request as ready for review December 22, 2023 15:07
Copy link
Member

@jGaboardi jGaboardi left a comment

Choose a reason for hiding this comment

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

Looking good.

libpysal/graph/base.py Outdated Show resolved Hide resolved
libpysal/graph/base.py Outdated Show resolved Hide resolved
@sjsrey
Copy link
Member

sjsrey commented Dec 22, 2023

#668 (comment)

@martinfleis martinfleis changed the title ENH: add fill_diagonal on Graph ENH: include add_self_loops (formerly fill_diagonal) in Graph Dec 22, 2023
Copy link
Member

@jGaboardi jGaboardi left a comment

Choose a reason for hiding this comment

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

Will the terminology in this PR be affected by the discussion in #668?

libpysal/graph/base.py Outdated Show resolved Hide resolved
libpysal/graph/base.py Outdated Show resolved Hide resolved
libpysal/graph/base.py Outdated Show resolved Hide resolved
@martinfleis
Copy link
Member Author

Will the terminology in this PR be affected by the discussion in #668?

Yes. I'll update this PR as soon we have an agreement.

@martinfleis
Copy link
Member Author

Updated as per #668. Check the docstring please if it makes sense.

libpysal/graph/base.py Outdated Show resolved Hide resolved
libpysal/graph/base.py Outdated Show resolved Hide resolved
libpysal/graph/base.py Outdated Show resolved Hide resolved
libpysal/graph/base.py Outdated Show resolved Hide resolved
libpysal/graph/base.py Outdated Show resolved Hide resolved
libpysal/graph/base.py Outdated Show resolved Hide resolved
martinfleis and others added 2 commits December 29, 2023 14:28
Co-authored-by: James Gaboardi <jgaboardi@gmail.com>
libpysal/graph/base.py Outdated Show resolved Hide resolved
Co-authored-by: James Gaboardi <jgaboardi@gmail.com>
@jGaboardi jGaboardi self-requested a review December 29, 2023 15:28
@jGaboardi
Copy link
Member

Shall we update the title of the issue to reflect changes in terminology?

@martinfleis martinfleis changed the title ENH: include add_self_loops (formerly fill_diagonal) in Graph ENH: include assign_self_weight (formerly fill_diagonal) in Graph Dec 29, 2023
@martinfleis martinfleis merged commit 57aa58c into pysal:main Dec 29, 2023
11 checks passed
@martinfleis martinfleis deleted the fill_diagonal branch December 29, 2023 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fill diagonal in graph
4 participants