ENH:force_ccw: generalized vectorized shapely.ops.orient; is_ccw: add fallback for GEOS < 3.7

[idanmiara]

Closes #1366
Closes #1684
Closes #1696 (mostly)

[idanmiara]

I'm wondering if I could generalize the decorator to work on any object type, not just on Geometry.
For this I need to replace isinstance(geoms, Geometry) in the decorators with a function like np.isscalar(geoms) but that works also for objects. np.isscalar returns False for Geometry or other objects. Any ideas ?

[jorisvandenbossche]

I'm wondering if I could generalize the decorator to work on any object type, not just on Geometry.

What would be the goal / use case of that?

In general, I am not sure if it is needed to already generalize this with a decorator, if we for now only have one place where we use this (and as long as we don't have plans to use it elsewhere as well).

---

Some other thoughts:

• If we add a vectorized version, we should probably directly add this as a top-level function as well, to be consistent with all other vectorized functions.
• If we add it as a new top-level function, we should discuss the naming, though. We are now using orient, but we generally try to follow the PostGIS naming (unless there is a good reason to deviate), in which case it would be something like force_ccw or force_polygon_ccw.
• If we provide a vectorized version, it would be nice to directly do this as a fast function in C (ufunc.c). I think that should be doable (without anything being added to GEOS upstream) and could mimic what we have for force_2d (that is also a custom implementation on our side, and forcing (c)cw would actually be simpler). If you would be interested in looking at this as well, I am happy to give some more pointers.

[coveralls]

Pull Request Test Coverage Report for Build 4026379876

• 42 of 46 (91.3%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.3%) to 87.093%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
shapely/decorators.py	8	12	66.67%

Totals
Change from base Build 4026134479:	0.3%
Covered Lines:	2409
Relevant Lines:	2766

---

💛 - Coveralls

[idanmiara]

What would be the goal / use case of that?

No specific other use case at the moment, just thought it would be a cleaner solution.

In general, I am not sure if it is needed to already generalize this with a decorator, if we for now only have one place where we use this (and as long as we don't have plans to use it elsewhere as well).

I think that could be a nice, easy to implement vectorize method to other functions with a single geometry as the first argument (for example, all the functions in affinity.py). I could have used it for the old transform function, but ended up using something a bit more optimized for that use case.

If we add it as a new top-level function, we should discuss the naming, though. We are now using orient, but we generally try to follow the PostGIS naming (unless there is a good reason to deviate), in which case it would be something like force_ccw or force_polygon_ccw.

I don't have an opinion about the naming of the function. If you want to pick one I'd be happy to put as a top level function and put it also under the old name.module for backwards compatibility.

If you would be interested in looking at this as well, I am happy to give some more pointers.

Thanks! Unfortunately, currently I don't have the time to go into this.

[idanmiara]

Some other thoughts:

• If we add a vectorized version, we should probably directly add this as a top-level function as well, to be consistent with all other vectorized functions.
• If we add it as a new top-level function, we should discuss the naming, though. We are now using orient, but we generally try to follow the PostGIS naming (unless there is a good reason to deviate), in which case it would be something like force_ccw or force_polygon_ccw.

Done.
I could cleanup the commit history if you approve this.

[idanmiara]

I've further improved orient and force_ccw using added new functions reversed_conditioned and using is_ccw.
since reversed and is_ccw are only available with GEOS >= 3.7 and the original orient did not have a GEOS minimum version, I've added a fallback implementation to both reversed and is_ccw, now all the tests for these functions also pass with GEOS < 3.7.

The new orient/force_ccw also support LinearRing (before it retuned the geometry unchanged), thus the relevant test was updated.

If you approve these additions but require smaller PRs for review I could split this PR.

[jorisvandenbossche]

Thanks for further working on this!

since reversed and is_ccw are only available with GEOS >= 3.7 and the original orient did not have a GEOS minimum version, I've added a fallback implementation to both reversed and is_ccw, now all the tests for these functions also pass with GEOS < 3.7.

In general, I think we shouldn't bother too much with custom code to support such an old GEOS version. It's fine that some features just raise an error that the GEOS version is too old, and we are probably going to require GEOS 3.7 as a minimum version in the near future anyway (#1000)

I could cleanup the commit history if you approve this.

No need to clean up commit histories, we squash on merge anyway.

[idanmiara]

Thanks for further working on this!

Thanks for your review :)

since reversed and is_ccw are only available with GEOS >= 3.7 and the original orient did not have a GEOS minimum version, I've added a fallback implementation to both reversed and is_ccw, now all the tests for these functions also pass with GEOS < 3.7.

In general, I think we shouldn't bother too much with custom code to support such an old GEOS version. It's fine that some features just raise an error that the GEOS version is too old, and we are probably going to require GEOS 3.7 as a minimum version in the near future anyway (#1000)

OK, Would you suggest I'll remove the fallback implementations for reverse and is_ccw and mark force_ccw (and orient) for GEOS >= 3.7 ?

I think that till GEOS 3.7 is the minimum we could keep the is_ccw fallback/workaround and remove the weird is_ccw_impl workaround (I will make a separate PR, if you like this idea)

I could cleanup the commit history if you approve this.

No need to clean up commit histories, we squash on merge anyway.

Yes for simple PRs I agree that that's the way to go.
If the PR is a bit more complex and the commit history makes sense I'd advise to rebase and keep the history.

[idanmiara]

@jorisvandenbossche I think I've fixed everything as per your comments

[idanmiara]

@jorisvandenbossche @kylebarron @brendan-ward I think this is ready for review.

[idanmiara]

rebased to resolve conflicts with main

[idanmiara]

@jorisvandenbossche @kylebarron @brendan-ward
rebased to resolve conflicts with main
would you like to review?

[idanmiara]

@jorisvandenbossche any further thoughts about this PR?