Skip to content

Deprecate ctypes attribute and array interface#955

Merged
sgillies merged 4 commits intoshapely:masterfrom
jorisvandenbossche:deprecate-array-interface
Aug 11, 2020
Merged

Deprecate ctypes attribute and array interface#955
sgillies merged 4 commits intoshapely:masterfrom
jorisvandenbossche:deprecate-array-interface

Conversation

@jorisvandenbossche
Copy link
Copy Markdown
Member

@jorisvandenbossche jorisvandenbossche commented Aug 5, 2020

Deprecating the ctypes attribute and the array_interface.

See https://github.com/shapely/shapely-rfc/pull/1/files#diff-554e526a1f0fe104169383c013e37a7cR360 for the rationale

The array interface is kept intact for the CoordinateSequence, so occurence of np.array(geom) can be replaced with np.array(geom.coords).
(we should eventually implement this differently (not using ctypes), but that can be left for later or even for Shapely 2.0)

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 5, 2020

Coverage Status

Coverage increased (+0.5%) to 82.581% when pulling 3a0cef0 on jorisvandenbossche:deprecate-array-interface into 24a9bf7 on Toblerity:master.

@jorisvandenbossche jorisvandenbossche changed the title Deprecate ctypes attribute Deprecate ctypes attribute and aray interface Aug 6, 2020
@jorisvandenbossche jorisvandenbossche changed the title Deprecate ctypes attribute and aray interface Deprecate ctypes attribute and array interface Aug 6, 2020
@jorisvandenbossche
Copy link
Copy Markdown
Member Author

@sgillies @tomplex this should be ready for review

@mwtoews mwtoews added this to the 1.8 milestone Aug 7, 2020
Comment on lines +43 to +45
coords = geometry.LineString([[12, 34], [56, 78]]).coords
with pytest.warns(ShapelyDeprecationWarning, match="ctypes"):
coords.ctypes
Copy link
Copy Markdown
Contributor

@sgillies sgillies Aug 9, 2020

Choose a reason for hiding this comment

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

@jorisvandenbossche would the following suffice?

Suggested change
coords = geometry.LineString([[12, 34], [56, 78]]).coords
with pytest.warns(ShapelyDeprecationWarning, match="ctypes"):
coords.ctypes
with pytest.warns(ShapelyDeprecationWarning, match="ctypes"):
geometry.LineString().coords.ctypes

I feel like the unused coordinate values are slightly distracting. Or maybe test the CoordinateSequence class only?

Also, what would you think about a minimal docstring for the test?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a docstring (and also fixed the test name, that was a copy/paster error).

geometry.LineString().coords.ctypes actually doesn't work, because for an empty geometry the coords attribute returns an empty list instead of a CoordinateSequence object (to me this feels a bit like a bug, but is done on purpose in the code).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree about the bug.

import numpy as np

line = LineString(((1.0, 2.0), (3.0, 4.0)))
with pytest.warns(ShapelyDeprecationWarning, match="array interface"):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Today I learned about the match keyword argument! I'm going to use this more often.

Copy link
Copy Markdown
Contributor

@sgillies sgillies left a comment

Choose a reason for hiding this comment

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

Seems right to me. I left two suggestions inline, one about unnecessary coordinate args for geometry constructors, and one about docstrings. I won't insist in either case. The docstrings are not absolutely necessary since these are transitional tests.

Copy link
Copy Markdown
Contributor

@tomplex tomplex left a comment

Choose a reason for hiding this comment

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

I don't have much to add here - it all looks good to me.

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.

5 participants