-
Notifications
You must be signed in to change notification settings - Fork 80
[WIP] shared perimeter-weighted contiguity #507
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
Conversation
@sjsrey the last commit implements the logic we talked about today. I can add a test case, or we could have you sanity-check again? also not too sure what that new kwarg should be called |
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
ive got a notebook to go with this, but running into issues when i create a perimeter-weighted W with cc: @martinfleis @ljwolf |
import geopandas as gpd
from libpysal.examples import load_example
from libpysal.weights import Rook
us = gpd.read_file(examples.get_path("us48.shp"))
old = Rook.from_dataframe(us)
new = Rook.from_dataframe(us.set_index('STATE_FIPS'), use_index=True) i thnk the neighbor relationships here are correct, but the indices get screwed up (which causes issues for the perimeter weights, as they rely on a join) |
Did I mess up #477? |
cant say for certain, but i think all of that is ok. I think we may just need to update the to_adjlist method |
im not sure whether its the this notebook runs fine as-is. But, if i explicitly set the index to something like STATE_FIPS in cell 25, then |
It could be due to libpysal/libpysal/weights/weights.py Line 435 in 3a09c42
which we have not yet deprecated. |
Maybe changing: libpysal/libpysal/weights/weights.py Lines 434 to 440 in 3a09c42
to
? |
I think the |
that gets the alignment back in shape so the weights are correct, but we lose the index itself (in the updated example 'focal' is an integer sequence, not fips codes) |
This is because We also don't have a way back to the df from We could get the In and instead do something like:
But I think we should get @ljwolf's view on these issues. |
got it so in the new api, we take the |
This was a good catch, as it raises a number of issues that are related to the planned deprecation. |
if we are getting rid of |
i think we also need to store (and expose publicly) the |
i think #511 solves the core issue, so this is probably ready as well. |
To make sure, this has already landed in |
yup, this is stale |
) | ||
|
||
# Putting it back to a matrix | ||
if perimeter_standardize: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljwolf do we want to keep the option to standardize when the boundary isnt exhausted? (so the denom is boundary_i instead of \sum{sharedboundary_ij})?
supercedes #506 ; resolves #80