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

Feat: Vectorized orient function #1684

Closed
kylebarron opened this issue Dec 23, 2022 · 8 comments · May be fixed by #1690
Closed

Feat: Vectorized orient function #1684

kylebarron opened this issue Dec 23, 2022 · 8 comments · May be fixed by #1690

Comments

@kylebarron
Copy link
Contributor

Expected behavior and actual behavior.

It looks like the shapely.geometry.polygon.orient function is not yet vectorized and only works with a single polygon object

Steps to reproduce the problem.

import shapely
xmin = [0, 1]
ymin = [3, 4]
xmax = [5, 6]
ymax = [7, 8]
geoms = shapely.box(xmin, ymin, xmax, ymax, ccw=False)
shapely.geometry.polygon.orient(geoms)
# AttributeError: 'numpy.ndarray' object has no attribute 'exterior'

Operating system

MacOS

Shapely version and provenance

2.0.0 from pip

@idanmiara
Copy link
Contributor

Hi @kylebarron
Please use the new vectorized function shaply.ops.orient added in #1690

@kylebarron
Copy link
Contributor Author

It looks like shapely.ops.orient is already vectorized as of 2.0, and additionally is already much faster than a python for loop (as we'd expect, so not sure if #1690 is necessary?):

import shapely
xmin = [0, 1]
ymin = [3, 4]
xmax = [5, 6]
ymax = [7, 8]
geoms = shapely.box(xmin, ymin, xmax, ymax, ccw=False)
big_geoms = list(geoms) * 1000

%timeit shapely.ops.orient(big_geoms)
# 313 ns ± 7.58 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

%timeit [shapely.ops.orient(geom) for geom in big_geoms]
# 244 ms ± 4.66 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

I didn't know shapely.ops.orient existed; I'll move this to a doc request instead of a feature request. When I search for orient in the doc website, the only API doc result is for shapely.geometry.polygon.orient. Is there an API doc page in the website for everything in shapely.ops? Or is it laid out by intention, not by python module

@kylebarron kylebarron changed the title Feat: Vectorized orient function Doc: Document orient function moved to shapely.ops.orient Dec 25, 2022
@idanmiara
Copy link
Contributor

It looks like shapely.ops.orient is already vectorized as of 2.0

Nope. It's just returns the original geom since it's not supported. just try your original test on it.

Is there an API doc page in the website for everything in shapely.ops? Or is it laid out by intention

There are many duplicated functions from in shapely 2 with fragmented documentation.
I'm working on improving the docs #1688

@kylebarron
Copy link
Contributor Author

It's just returns the original geom since it's not supported

Oh that seems really bad: it's silently returning results with no indication that the orient didn't happen

@kylebarron kylebarron changed the title Doc: Document orient function moved to shapely.ops.orient Feat: Vectorized orient function Dec 25, 2022
@idanmiara
Copy link
Contributor

idanmiara commented Dec 25, 2022

It's just returns the original geom since it's not supported

Oh that seems really bad: it's silently returning results with no indication that the orient didn't happen

I think that the idea was that if the input is not BaseMultipartGeometry and not Polygon then it's a geometry that is oriented (not sure if that's true).
Now if you pass something that's not a Geometry, like a geometry array, it could be considered as an invalid input thus the output is unexpected.

With #1690 it works also for geometry array.

@jorisvandenbossche
Copy link
Member

Yes, the existing shapely.ops.orient was indeed assuming you always pass a single geometry (and passing through anything that has no polygonal parts). But it is certainly unfortunate that it didn't at least check that it receives a geometry, to avoid such confusion.

Existing issue about providing a vectorized version of this: #1366

@jorisvandenbossche
Copy link
Member

Duplicate of #1366

@jorisvandenbossche jorisvandenbossche marked this as a duplicate of #1366 Dec 28, 2022
@idanmiara
Copy link
Contributor

idanmiara commented Jan 1, 2023

@kylebarron
Please see new implementation for force_ccw in #1690
I didn't time it, but it is probably a bit faster than the old orient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants