Skip to content

API: remove __geom__, keep _geom read-only access to GEOS pointer#1417

Merged
jorisvandenbossche merged 1 commit intoshapely:mainfrom
jorisvandenbossche:geos-pointer-attribute
Jun 19, 2022
Merged

API: remove __geom__, keep _geom read-only access to GEOS pointer#1417
jorisvandenbossche merged 1 commit intoshapely:mainfrom
jorisvandenbossche:geos-pointer-attribute

Conversation

@jorisvandenbossche
Copy link
Copy Markdown
Member

@jorisvandenbossche jorisvandenbossche commented Jun 13, 2022

Closes #999. This removes the __geom__ dunder attribute and keeps _geom (renaming the _ptr inherited from pygeos to _geom).

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 13, 2022

Pull Request Test Coverage Report for Build 2487695785

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 84.358%
Totals Coverage Status
Change from base Build 2456549977: -0.004%
Covered Lines: 2141
Relevant Lines: 2538
💛 - Coveralls

@mwtoews
Copy link
Copy Markdown
Member

mwtoews commented Jun 13, 2022

Looks great, two quick questions.

  1. Is this _geom correct?

    shapely/shapely/_geos.pxi

    Line 59 in 8d1911e

    cdef uintptr_t geos_geom = shapely_geom._geom

  2. Slightly related to this PR, should this _ptr also be renamed?

    shapely/src/strtree.c

    Line 1283 in 8d1911e

    {"_ptr", T_PYSSIZET, offsetof(STRtreeObject, ptr), READONLY,

jorisvandenbossche
jorisvandenbossche commented Jun 13, 2022
View reviewed changes
shapely/tests/test_io.py
class ShapelyGeometryMock:
def __init__(self, g):
self.g = g
self.__geom__ = g._ptr if hasattr(g, "_ptr") else g
Copy link
Copy Markdown
Member Author

@jorisvandenbossche jorisvandenbossche Jun 13, 2022

Choose a reason for hiding this comment

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

This was some leftover code from pygeos (testing it could convert shapely geometries), that could be cleaned-up (while removing all references of __geom__)

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

jorisvandenbossche commented Jun 13, 2022

  1. Is this _geom correct?

Ah, that's actually in a file that is no longer used. Cleaning that up separately -> #1419

2. Slightly related to this PR, should this _ptr also be renamed?

Yeah, I wondered the same. But the problem is that renaming it to _geom would not be correct since it is not a geometry pointer (but STRtree). For consistency we could rename it to something else than _ptr, but leaving it as _ptr also seems fine for me (it won't be exactly the same as _geom for the geometry class anyway)

brendan-ward
brendan-ward approved these changes Jun 13, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@brendan-ward brendan-ward 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 , looks good to me.

@jorisvandenbossche jorisvandenbossche mentioned this pull request Jun 19, 2022
Closed
@jorisvandenbossche jorisvandenbossche merged commit 0a11e6e into shapely:main Jun 19, 2022
@jorisvandenbossche jorisvandenbossche deleted the geos-pointer-attribute branch June 19, 2022 11:33
@mgrover1 mgrover1 mentioned this pull request Aug 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mwtoews mwtoews mwtoews approved these changes

@brendan-ward brendan-ward brendan-ward approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

API: read-only access to GEOS pointer (__geom__, _geom, _ptr)

4 participants

@jorisvandenbossche @coveralls @mwtoews @brendan-ward