Skip to content

Refactor strtree to not use lgeos / ctypes#1161

Merged
jorisvandenbossche merged 2 commits intoshapely:shapely-2.0from
jorisvandenbossche:refactor-2.0-strtree
Sep 30, 2021
Merged

Refactor strtree to not use lgeos / ctypes#1161
jorisvandenbossche merged 2 commits intoshapely:shapely-2.0from
jorisvandenbossche:refactor-2.0-strtree

Conversation

@jorisvandenbossche
Copy link
Copy Markdown
Member

This is for the shapely-2.0 branch, to do the equivalent of #1112 or #1064 (and ideally only gets done when one of those PRs is merged).

This shows how the lib.STRtree (the C object) will just contain primary keys, which are mapped to geometries (or other items) in the query method using the returned indices.

Copy link
Copy Markdown
Member

@snorfalorpagus snorfalorpagus left a comment

Choose a reason for hiding this comment

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

Added some minor comments.

I like the new query_items function. This is a real pain in the current API.

else:
return (STRtree, (self.geometries, ))

def query(self, geom):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This function now returns an np.array instead of a list which is causing the doctest to fail.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

docstring currently has "array or list of geometry objects.". Perhaps it should be updated to say just "array".

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.

Ah, yes, updated the docstrings.

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

@snorfalorpagus

I like the new query_items function. This is a real pain in the current API.

That's actually already introduced in the main branch (and so will be in shapely 1.8), coming from #1112 / #1064

Related to the missing docstrings and tests, they were also introduced in #1112, so I need to rebase this branch to pull those in (I made this branch with just similar code changes before #1112 was merged).

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 20, 2021

Coverage Status

Coverage decreased (-0.2%) to 77.248% when pulling 5c04e37 on jorisvandenbossche:refactor-2.0-strtree into f9743ea on Toblerity:shapely-2.0.

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

I updated the shapely-2.0 branch with the changes of latest master, and so also updated this branch (so docstrings and tests are now already included)

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

Note that this PR disables the exclusive keyword again in the nearest_item/nearest_geom methods (which was introduced in #1166), since pygeos does not yet support this.
Of course ideally we fix this before an actual 2.0 release, but I would prefer to not keep up this PR until it's supported in the pygeos version.

Copy link
Copy Markdown
Member

@mwtoews mwtoews left a comment

Choose a reason for hiding this comment

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

Admittedly, I don't know STRTree very well, but I've tested this locally and it seems to be in working order.

Perhaps the docstrings could be updated to say if an array is returned from (e.g.) query. Also, should query and nearest be at least deprecated? (These are still in consideration to be removed for 2.0)

Is there an issue in pygeos for the exclusive keyword? It hopefully shouldn't be too difficult to implement.

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

Is there an issue in pygeos for the exclusive keyword? It hopefully shouldn't be too difficult to implement.

Didn't do that yet (will do now). Somewhat related, I did just open an issue about some other API incompatibilities between the pygeos and Shapely STRtree: pygeos/pygeos#398

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

Opened pygeos/pygeos#400 for the exclusive keyword.

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

jorisvandenbossche commented Sep 30, 2021

Also, should query and nearest be at least deprecated? (These are still in consideration to be removed for 2.0)

Not fully sure (it's indeed duplicating the other method now), but I think that's a separate issue to discuss (and maybe not critical for 1.8? We can also still deprecate it in 2.0)

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

Since this is only replacing the internals of the tree with pygeos (and not changing any API), I am going to merge this, so I can update #1163

@jorisvandenbossche jorisvandenbossche merged commit da8fd74 into shapely:shapely-2.0 Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants