expand graph.Graph constructors and other functionality #544

martinfleis · 2023-08-07T21:27:38Z

Very much WIP but wanted to open this for discussion. This partially handles isolates but not always. Distance band and knn are used to compute distance matrix that is then (optionally) passed via kernel.

@ljwolf I am not sure if this is the implementation you had in mind, but this seems to also work well.

includes:

Graph.build_distance_band
reworked kernels

edit: Also includes:

Graph.build_block_contiguity()
Parquet IO as graph.read_parquet and Graph.to_parquet mirroring (geo)pandas
Graph.lag()
__getitem__

martinfleis · 2023-08-07T21:28:44Z

libpysal/graph/_kernel.py

+    dist = tree.sparse_distance_matrix(tree, threshold, output_type="ndarray")
+    return sparse.csr_array((dist["v"], (dist["i"], dist["j"])))


This is a trick how to get a sparse array instead of a sparse matrix out of sparse_distance_matrix. I suppose that scipy will have sparse_distance_array eventually but this is fairly cheap anyway.

martinfleis · 2023-08-07T21:30:07Z

libpysal/graph/base.py

+        adjacency["weight"] = (
+            adjacency["weight"].fillna(0).replace([np.inf, -np.inf], 0)
+        )  # handle isolates
+
+        # drop diagnoal
+        counts = adjacency.index.value_counts()
+        no_isolates = counts[counts > 1]
+        adjacency = adjacency[
+            ~(
+                adjacency.index.isin(no_isolates.index)
+                & (adjacency.index == adjacency.neighbor)
+            )
+        ]


These bits should ideally be handled within _kernel and properly. This does not always work.

codecov · 2023-08-07T21:42:16Z

Codecov Report

Merging #544 (4d01f41) into geographs (c6d8e2a) will increase coverage by 0.3%.
Report is 2 commits behind head on geographs.
The diff coverage is 97.3%.

@@             Coverage Diff             @@
##           geographs    #544     +/-   ##
===========================================
+ Coverage       80.8%   81.1%   +0.3%     
===========================================
  Files            125     127      +2     
  Lines          14351   14549    +198     
===========================================
+ Hits           11595   11803    +208     
+ Misses          2756    2746     -10

Files Changed	Coverage Δ
libpysal/graph/_utils.py	`42.7% <ø> (ø)`
libpysal/graph/_parquet.py	`84.0% <84.0%> (ø)`
libpysal/graph/_kernel.py	`69.2% <91.3%> (+6.6%)`	⬆️
libpysal/graph/_contiguity.py	`97.3% <100.0%> (+0.4%)`	⬆️
libpysal/graph/_spatial_lag.py	`100.0% <100.0%> (ø)`
libpysal/graph/base.py	`98.2% <100.0%> (+0.3%)`	⬆️
libpysal/graph/tests/test_base.py	`100.0% <100.0%> (ø)`
libpysal/graph/tests/test_builders.py	`100.0% <100.0%> (ø)`
libpysal/graph/tests/test_contiguity.py	`100.0% <100.0%> (ø)`
libpysal/graph/tests/test_spatial_lag.py	`100.0% <100.0%> (ø)`

... and 1 file with indirect coverage changes

ljwolf · 2023-08-07T22:15:20Z

Yep, this is exactly what I had in mind!

martinfleis · 2023-08-10T18:50:12Z

This one is ready for review. Not everything properly works (isolates in kernel-based stuff is sketchy in some and plain wrong in other cases) but I'd leave that for follow-ups. I've left TODO comments for those.

I will be leaving for a week on Saturday and will not be able to respond to anything, so it would be best if this is merged by Saturday morning (my time) to ensure I am not a blocker.

Tests are mostly checking dtypes and expected shapes but not necessarily values and all possible paths. That will need to be done as follow-ups, especially for constructors.

jGaboardi

Looks good as far as I can tell, considering your comments above.

libpysal/graph/base.py

Co-authored-by: James Gaboardi <jgaboardi@gmail.com>

jGaboardi · 2023-08-10T22:04:10Z

libpysal/graph/_kernel.py

+    neighbor_ix_flat = neighbor_ix.flatten()
+    D_linear_flat = D_linear.flatten()
+    if metric == "haversine":
+        D_linear_flat * 6371  # express haversine distances in kilometers


Should we add a note to the docstrings of exposed functions that the returned haversine distance is in kilometers (as opposed to meters)?

For haversine, we need more than that. It requires lat lan coords so we need to check if the gdf is in the correct CRS (or None) and if an array is given, that it is bounded by -180,180 and -90,90. I'll add this as a todo note.

Yes, I implemented this so it would do the "right" thing when a geographically-literate user sends lon,lat coordinates, since sklearn haversine expects coordinates in radian lat,lon.

Thinking of it now, though, it'd be great to have a way to say "take my coordinates as is and use haversine" since deg2rad() will instantiate a new array if the input coordinates are a numpy.memmap... maybe the way around this is to allow for tree-based input in coordinates.

ljwolf · 2023-08-11T09:14:32Z

libpysal/graph/_kernel.py

+    neighbor_ix_flat = neighbor_ix.flatten()
+    D_linear_flat = D_linear.flatten()
+    if metric == "haversine":
+        D_linear_flat * 6371  # express haversine distances in kilometers


Yes, I implemented this so it would do the "right" thing when a geographically-literate user sends lon,lat coordinates, since sklearn haversine expects coordinates in radian lat,lon.

Thinking of it now, though, it'd be great to have a way to say "take my coordinates as is and use haversine" since deg2rad() will instantiate a new array if the input coordinates are a numpy.memmap... maybe the way around this is to allow for tree-based input in coordinates.

ljwolf · 2023-08-11T09:19:17Z

libpysal/graph/base.py

+                ids=ids,
+                bandwidth=alpha,
+            )
+        sp.setdiag(0)


This is not necessarily valid, but I can understand why this is here. Many kernel weights require self-weighting. It would be good to provide a way to control this.

This was likely related to some isolate handling. That is still to-do as a follow up so I'll just add a note here to reconsider this.

ljwolf · 2023-08-11T09:20:20Z

libpysal/graph/base.py

-                    self._adjacency.index.map(self.id2i),
-                    self._adjacency.neighbor.map(self.id2i),
+                    self._adjacency.index.map(self._id2i),
+                    self._adjacency.neighbor.map(self._id2i),


is id2i how we're doing the lookups now from name to index?

yes. do you have a better suggestion?

No no! I was just wondering, given I'll need a way to deal with this when re-ordering the adjacency list after expanding cliques.

2 things here from my end:

Regarding the public vs. private discussion for id2i, was this talked about anywhere else besides here? Since id2i is currently exposed in the W implementation, I am curious what the argument is for making it private in the new Graph implementation.

Also, I wanted to bring up the idea again to discuss the potential usefulness of an i2id property (or _i2id if we stick with the current private schema). Currently that translated lookup is only used within asymmetry, but I am wondering if anyone can think of other situations where it might be helpful? If we will be using a translated lookup anywhere else, then another property is surely warranted IMHO.

xref: BUG: weights.W.asymmetry does not support custom ids #540 (comment)

ljwolf · 2023-08-11T17:04:25Z

I would like to merge this so that I can rebase #548... every triangulation can pass through kernel, so we need the kernel implementation stabilised before I can finish up the triangulation stuff.

@jGaboardi your review doesn't have any further outstanding requests, right? just questions about id2i? If so, can I merge this?

martinfleis · 2023-08-11T17:09:35Z

@ljwolf Go ahead. We can open issues for API discussions.

martinfleis added 2 commits August 7, 2023 21:18

add distance_band

5086103

channel distance band through kernel

18c2a6d

martinfleis added WIP Work in progress, do not merge. Discussion only. graph labels Aug 7, 2023

martinfleis self-assigned this Aug 7, 2023

martinfleis commented Aug 7, 2023

View reviewed changes

martinfleis mentioned this pull request Aug 7, 2023

graph.Graph build_* constructors #542

Merged

fix isolates in distance band

bb4f47c

lag, parquet IO, __getitem__, block placeholder

467a987

martinfleis changed the title ~~expand graph.Graph kernel-based constructors~~ expand graph.Graph constructors Aug 10, 2023

martinfleis added 4 commits August 10, 2023 18:20

docstrings, optional pyarrow

172ca3c

fix test

fc25077

block contiguity

35da0cd

add tests

5ec5106

martinfleis changed the title ~~expand graph.Graph constructors~~ expand graph.Graph constructors and other functionality Aug 10, 2023

martinfleis removed the WIP Work in progress, do not merge. Discussion only. label Aug 10, 2023

martinfleis marked this pull request as ready for review August 10, 2023 18:45

martinfleis requested review from sjsrey, ljwolf, knaaptime and jGaboardi August 10, 2023 18:45

to-dos

3ddbf94

jGaboardi reviewed Aug 10, 2023

View reviewed changes

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 3 commits August 10, 2023 22:03

Apply suggestions from code review

4b5d01d

Co-authored-by: James Gaboardi <jgaboardi@gmail.com>

test block_contiguity

04458b5

test parquet without meta

7d97c0c

test lag

ea2d989

jGaboardi reviewed Aug 10, 2023

View reviewed changes

haversine todo note

009af6c

ljwolf reviewed Aug 11, 2023

View reviewed changes

comment on diag

4d01f41

ljwolf merged commit bd228c7 into pysal:geographs Aug 11, 2023
9 checks passed

martinfleis deleted the more_constructors branch August 11, 2023 17:23

ljwolf mentioned this pull request Aug 11, 2023

Intercept Coincident points #548

Merged

5 tasks

jGaboardi mentioned this pull request Aug 11, 2023

2 considerations for id2i in the new Graph class #549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expand graph.Graph constructors and other functionality #544

expand graph.Graph constructors and other functionality #544

martinfleis commented Aug 7, 2023 •

edited

Loading

martinfleis Aug 7, 2023

martinfleis Aug 7, 2023

codecov bot commented Aug 7, 2023 •

edited

Loading

ljwolf commented Aug 7, 2023

martinfleis commented Aug 10, 2023

jGaboardi left a comment

jGaboardi Aug 10, 2023

martinfleis Aug 11, 2023

ljwolf Aug 11, 2023

ljwolf Aug 11, 2023

ljwolf Aug 11, 2023

martinfleis Aug 11, 2023

ljwolf Aug 11, 2023

martinfleis Aug 11, 2023

ljwolf Aug 11, 2023

jGaboardi Aug 11, 2023

ljwolf commented Aug 11, 2023

martinfleis commented Aug 11, 2023

		dist = tree.sparse_distance_matrix(tree, threshold, output_type="ndarray")
		return sparse.csr_array((dist["v"], (dist["i"], dist["j"])))

expand graph.Graph constructors and other functionality #544

expand graph.Graph constructors and other functionality #544

Conversation

martinfleis commented Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2023 • edited Loading

Codecov Report

ljwolf commented Aug 7, 2023

martinfleis commented Aug 10, 2023

jGaboardi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljwolf commented Aug 11, 2023

martinfleis commented Aug 11, 2023

martinfleis commented Aug 7, 2023 •

edited

Loading

codecov bot commented Aug 7, 2023 •

edited

Loading