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.

Sign up for GitHub

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

Jump to bottom

Warn about deprecation of sindex.intersection objects parameter #1440

Merged
merged 4 commits into from Jun 7, 2020
Merged

Warn about deprecation of sindex.intersection objects parameter #1440

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0c66b6
warn about deprecation of objects parameter
adriangb May 21, 2020
0b12b2f
add warning check
adriangb May 22, 2020
de95e9a
PR feedback
adriangb May 22, 2020
88099ea
fix tests warning match
adriangb May 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 32 additions & 0 deletions geopandas/sindex.py
View file
Open in desktop
@@ -1,4 +1,5 @@
from collections import namedtuple
from warnings import warn

from shapely.geometry.base import BaseGeometry
import pandas as pd
Expand Down Expand Up @@ -243,6 +244,28 @@ def query_bulk(self, geometry, predicate=None, sort=False):
input_geometry_index.extend([i] * len(res))
return np.vstack([input_geometry_index, tree_index])

def intersection(self, coordinates, objects=False):
"""Find tree geometries that intersect the input coordinates.

Parameters
----------
coordinates : sequence or array
Sequence of the form (min_x, min_y, max_x, max_y)
to query a rectangle or (x, y) to query a point.
objects : True or False
adriangb marked this conversation as resolved.
Show resolved Hide resolved
If True, return the label based indexes. If False, integer indexes
are returned.
adriangb marked this conversation as resolved.
Show resolved Hide resolved
"""
if objects:
warn(
"A future version of GeoPandas will deprecate the `objects`"
Copy link
Member

Choose a reason for hiding this comment

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

How about

'objects' is deprecated and will be replaced by a different interface to labels in the sindex geometries in an upcoming release.

I don't know that the "If you use this option" is necessary here; this doesn't get added to other deprecation notices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thoughts were that we do not really know who uses this interface and why. I thought it would be good to get some issue reports that might help shape what that future implementation of loc/label based indexing will look like, or if it is needed at all.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can just say it will be removed in the future. But it might be good to indicate what to do instead (how to achieve the same behaviour without using the keyword), so it's clear for users how to replace their usage of it.

Copy link
Contributor Author

@adriangb adriangb May 22, 2020 •
edited

Choose a reason for hiding this comment

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

So something like:

`objects` is deprecated and will be removed in a future version. Instead, use `iloc` to index your GeoSeries/GeoDataFrame using integer indexes returned by `intersection`.

?

" parameter. If you use this option and would like it preserved,"
" please open an issue at https://github.com/geopandas/geopandas"
" and help us understand you use case.",
FutureWarning,
)
return super().intersection(coordinates, objects)

@property
def size(self):
return len(self.leaves()[0][1])
Expand Down Expand Up @@ -407,6 +430,15 @@ def intersection(self, coordinates, objects=False):
If True, return the label based indexes. If False, integer indexes
are returned.
adriangb marked this conversation as resolved.
Show resolved Hide resolved
"""
if objects:
warn(
"A future version of GeoPandas will deprecate the `objects` "
" parameter. If you use this option and would like it preserved, "
" please open an issue at https://github.com/geopandas/geopandas"
" and help us understand you use case.",
FutureWarning,
)

# convert bounds to geometry
# the old API uses tuples of bound, but pygeos uses geometries
try:
Expand Down
30 changes: 25 additions & 5 deletions geopandas/tests/test_sindex.py
View file
Open in desktop
Expand Up @@ -105,7 +105,11 @@ def setup_method(self):
def test_sindex(self):
self.df.crs = "epsg:4326"
assert self.df.sindex.size == 5
hits = list(self.df.sindex.intersection((2.5, 2.5, 4, 4), objects=True))
with pytest.warns(
FutureWarning, match="A future version of GeoPandas will deprecate"
):
# TODO: remove warning check once deprecated
hits = list(self.df.sindex.intersection((2.5, 2.5, 4, 4), objects=True))
assert len(hits) == 2
assert hits[0].object == 3

Expand Down Expand Up @@ -133,21 +137,33 @@ def setup_method(self):
def test_merge_geo(self):
# First check that we gets hits from the boros frame.
tree = self.boros.sindex
hits = tree.intersection((1012821.80, 229228.26), objects=True)
with pytest.warns(
FutureWarning, match="A future version of GeoPandas will deprecate"
):
# TODO: remove warning check once deprecated
hits = tree.intersection((1012821.80, 229228.26), objects=True)
res = [self.boros.loc[hit.object]["BoroName"] for hit in hits]
assert res == ["Bronx", "Queens"]

# Check that we only get the Bronx from this view.
first = self.boros[self.boros["BoroCode"] < 3]
tree = first.sindex
hits = tree.intersection((1012821.80, 229228.26), objects=True)
with pytest.warns(
FutureWarning, match="A future version of GeoPandas will deprecate"
):
# TODO: remove warning check once deprecated
hits = tree.intersection((1012821.80, 229228.26), objects=True)
res = [first.loc[hit.object]["BoroName"] for hit in hits]
assert res == ["Bronx"]

# Check that we only get Queens from this view.
second = self.boros[self.boros["BoroCode"] >= 3]
tree = second.sindex
hits = tree.intersection((1012821.80, 229228.26), objects=True)
with pytest.warns(
FutureWarning, match="A future version of GeoPandas will deprecate"
):
# TODO: remove warning check once deprecated
hits = tree.intersection((1012821.80, 229228.26), objects=True)
res = ([second.loc[hit.object]["BoroName"] for hit in hits],)
assert res == ["Queens"]

Expand All @@ -156,7 +172,11 @@ def test_merge_geo(self):
assert len(merged) == 5
assert merged.sindex.size == 5
tree = merged.sindex
hits = tree.intersection((1012821.80, 229228.26), objects=True)
with pytest.warns(
FutureWarning, match="A future version of GeoPandas will deprecate"
):
# TODO: remove warning check once deprecated
hits = tree.intersection((1012821.80, 229228.26), objects=True)
res = [merged.loc[hit.object]["BoroName"] for hit in hits]
assert res == ["Bronx", "Queens"]

Expand Down