Skip to content

Deprecate asShape (CachingGeometryProxy adapter classes)#926

Merged
sgillies merged 3 commits intoshapely:masterfrom
jorisvandenbossche:deprecate-poxies
Jun 12, 2020
Merged

Deprecate asShape (CachingGeometryProxy adapter classes)#926
sgillies merged 3 commits intoshapely:masterfrom
jorisvandenbossche:deprecate-poxies

Conversation

@jorisvandenbossche
Copy link
Copy Markdown
Member

Deprecating the adapter classes that create geometry-like proxy objects with coordinates stored in external objects (xref shapely/shapely-rfc#1).

I again started with the Point geometry to check if the workflow looks good.

There are however multiple ways to create those objects. For points, you have the general asShape and asPoint, but in principle also the PointAdapter class constructor (but I don't know whether those are regarded as public API?).

The way I did it now is to add a deprecation warning to both asShape and asPoint, so the message can be specific. Bu there are certainly other ways to do it as well.


if geom_type == "point":
return asPoint(ob["coordinates"])
return _asPoint(ob["coordinates"])
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.

This is to avoid a duplicate warning coming from asPoint (the alternative is to only warn in asPoint with a more general warning message, and that warning will the also be seen when calling asShape)

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 6, 2020

Coverage Status

Coverage increased (+0.3%) to 82.096% when pulling 245986d on jorisvandenbossche:deprecate-poxies into ea25f8c on Toblerity:master.

@sgillies
Copy link
Copy Markdown
Contributor

sgillies commented Jun 7, 2020

Excellent, @jorisvandenbossche . I can't think of a reason not to eliminate the adapter classes in 2.0. There's a lot of code there that we won't want to maintain anymore and it only exists to support a kind of mutability.

How about warning from the adapter constructors, mentioning deprecation of the class and the factory functions (asShape, asPoint, &c)?

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

How about warning from the adapter constructors, mentioning deprecation of the class and the factory functions (asShape, asPoint, &c)?

OK, doing a warning only in the class + a general message so it is valid for all possible ways to construct it sounds good.

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

Updated the warning message and location. If that looks good, then will do the same for the other geometry types.

self.assertIsInstance(shape, MultiPolygonAdapter)
self.assertEqual(len(shape.geoms), 1)

def test_geointerface(self):
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.

I copied the test above, and adapted it to use shape instead of asShape, as there didn't seem a dedicated test for shape for all different geometry types.


@unittest.skipIf(not numpy, 'numpy not installed')
def test_multipoint(self):
arr = numpy.array([[1.0, 1.0, 2.0, 2.0, 1.0], [3.0, 4.0, 4.0, 3.0, 3.0]])
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.

Also here, copied the tests above but adapted using the normal constructor, to ensure we keep tests for this functionality when the adapter tests are removed.

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

@sgillies ok, updated for all geometry types. This is ready for review / merge.

@sgillies
Copy link
Copy Markdown
Contributor

Excellent!

@sgillies sgillies merged commit 97fc2c7 into shapely:master Jun 12, 2020
@jorisvandenbossche jorisvandenbossche deleted the deprecate-poxies branch June 13, 2020 13:37
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.

3 participants