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

Update of the min_threshold_dist_from_shapefile function to support geopandas objects directly. #436

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

Conversation

PhilipeRLeal
Copy link

  1. The justification for this PR is: The min_threshold_dist_from_shapefile function (from libpysal.utils) was updated so to support a geopandas GeoDataFrame or a geopandas.GeoSeries object directly. This Pull Request's update also ensured back-compatibility with the min_threshold_dist_from_shapefile elder behavior - of dealing with strings pointing towards shapefile filepaths.

Implications:

None

The get_points_array_from_gdf function was updated so to support its applicability to a geopandas GeoDataFrame or a geopandas.GeoSeries object directly.
@codecov
Copy link

codecov bot commented Oct 2, 2021

Codecov Report

Merging #436 (acbbb82) into master (a86520c) will increase coverage by 0.0%.
The diff coverage is 33.3%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #436   +/-   ##
======================================
  Coverage    79.7%   79.7%           
======================================
  Files         120     120           
  Lines       12710   12715    +5     
======================================
+ Hits        10128   10138   +10     
+ Misses       2582    2577    -5     
Impacted Files Coverage Δ
libpysal/weights/util.py 75.2% <33.3%> (-0.6%) ⬇️
libpysal/_version.py 40.7% <0.0%> (+2.7%) ⬆️

Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

I have very mixed feelings here.

The change in this PR is perfectly fine code-wise (just see the note in the code).

But API-wise, this is a bit messy but that is more for a general discussion among libpysal team. *from_shapefile should be a different thing than *from_dataframe. In the ideal case, we wouldn't need any *from_shapefile or *from_dataframe. We should assume that the input is GeoDataFrame and leave the file IO to geopandas.

There is a lot of legacy code in libpysal that should be cleaned and refactored based on geopandas and pygeos. Short term, we may just merge this assuming that these functions will eventually be deprecated anyway. But I would surely not go in the direction of shapefile==GeoDataFrame within our API.

libpysal/weights/util.py Outdated Show resolved Hide resolved
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
@PhilipeRLeal
Copy link
Author

PhilipeRLeal commented Oct 4, 2021

Dear @martinfleis,

thank's for the feedback. Your suggestion was completely right, and it has beeen accepted in this new commit.

SIncerely,

@ljwolf ljwolf added the weights label Oct 19, 2021
@martinfleis martinfleis changed the base branch from master to main February 27, 2023 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants