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

Raise ValueError for non-finite distance to buffer/offset_curve #1522

Merged
merged 1 commit into from
Oct 29, 2022

Conversation

mwtoews
Copy link
Member

@mwtoews mwtoews commented Sep 5, 2022

Similar to #1516 for maint-1.8, raise ValueError for non-finite distance values passed to buffer or offset_curve (aka parallel_offset) functions.

Previous behavior for shapely-2.0 did not crash (as it did for 1.8), but would be inconsistent:

  • distance float('inf') would raise "GEOSException: IllegalArgumentException: CGAlgorithmsDD::orientationIndex encountered NaN/Inf numbers"
  • distance float("nan") would not raise or warn, but would return None

@mwtoews mwtoews added this to the 2.0 milestone Sep 5, 2022
@coveralls
Copy link

coveralls commented Sep 5, 2022

Pull Request Test Coverage Report for Build 3204158908

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 84.612%

Totals Coverage Status
Change from base Build 3172352620: 0.03%
Covered Lines: 2150
Relevant Lines: 2541

💛 - Coveralls

@jorisvandenbossche
Copy link
Member

Previous behavior for shapely-2.0 did not crash (as it did for 1.8), but would be inconsistent:

On the other hand, now it is inconsistent with the top-level vectorized functions .. (I think it can certainly be fine to deviate in some cases in the Geometry subclasses, since it can be specialized for the scalar use case, but it should be a conscious decision)

@mwtoews
Copy link
Member Author

mwtoews commented Sep 5, 2022

Oh right, here it is. A few observations: the second parameter is either radius or width, while shapely 1.8, GEOS and JTS use distance. I should probably change this in this PR to distance for consistency.

Also one of the examples has buffer(line, float("nan")) is None, as noted at top. Should vectorized forms continue to return None on error (and be different than from base geometry), or change to raise ValueError? Is now a good time to introduce (e.g.) on_error="null" options?

Lastly, forgot to mention upstream libgeos/geos#663 which would change messaging/behavior.

@jorisvandenbossche
Copy link
Member

the second parameter is either radius or width, while shapely 1.8, GEOS and JTS use distance. I should probably change this in this PR to distance for consistency.

Ah, good catch, that's another inconsistency I didn't note in #1276. Using distance seems fine for me.

Should vectorized forms continue to return None on error (and be different than from base geometry), or change to raise ValueError? Is now a good time to introduce (e.g.) on_error="null" options?

My preference is that we keep returning None. In general when adding those to pygeos, we have been following the idea that None represents missing values for geometry data, and NaN for float data, and that we try to propagate missing values (instead of erroring) in the element-wise funcs.
(of course, NaN for float data is strictly speaking not a missing value (like "null" or "NA"), but in practice in the python ecosystem often treated that way given the limitation of numpy of not having actual nulls)

But at the same time, I think we could also certainly consider 1) being more strict for scalar geometry class methods, and 2) making the above behaviour configurable with some keyword.

@mwtoews
Copy link
Member Author

mwtoews commented Oct 28, 2022

I think this is ready to go, as it addresses single geometry operations which should behave differently than the vectorized functions with respect to NaN values (used for missing).

Further work with the vectorized functions should follow, as discussed above.

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Thanks!

@jorisvandenbossche jorisvandenbossche merged commit da27cdc into shapely:main Oct 29, 2022
@mwtoews mwtoews deleted the non-finite-distance branch October 29, 2022 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants