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

[FEA] Update cuspatial notebook to remove 31 polygon limitation workaround #343

Open
taureandyernv opened this issue Jan 20, 2021 · 9 comments
Labels
feature request New feature or request Needs Triage Need team to review and classify

Comments

@taureandyernv
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Previously, cuSpatial was limited to processing 31 polygons at a time. according to @thomcom , @trxcllnt and @cwharris did a great job removing this limitation. Now our notebook, https://github.com/rapidsai/cuspatial/blob/branch-0.18/notebooks/nyc_taxi_years_correlation.ipynb, needs to reflect that. The work around used needs to be removed.

Describe the solution you'd like
update notebook

@taureandyernv taureandyernv added Needs Triage Need team to review and classify feature request New feature or request labels Jan 20, 2021
@zhangjianting
Copy link
Contributor

zhangjianting commented Feb 1, 2021

I think the "old" point-in-polygon test API is still useful when the number of polygons is small for two reasons: 1) no need for indexing on points which could be both convenient and efficient. 2) For applications such as Video Analytics in Intelligent Transportation (ITS), the solution is a perfect match, as the number of polygons (Region of Interests) at an intersection is smaller than 32, i.e., sizeof(uint32_t). Point data may come in a stream mode which makes it difficult to index. This actually was the motivation to develop this point-in-polygon test API two years ago.
Instead of removing this API, I would suggest making better documentation and give users a choice.
@taureandyernv @thomcom , @trxcllnt and @cwharris

@thomcom
Copy link
Contributor

thomcom commented Feb 22, 2021

@taureandyernv how about we rename the old pip to point_in_polygon_bitmap and the new one to point_in_polygon_quadtree and update the docs, instead of deprecating like we worked on last week?

@taureandyernv
Copy link
Contributor Author

taureandyernv commented Feb 22, 2021

sure, @thomcom . Shall we put a notice on each of them that tells the user to use pip_bitmap (pip_bm) for small numbers of polygons and pip_qt for larger ones? Thanks for that great explanation @zhangjianting.

@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@thomcom
Copy link
Contributor

thomcom commented Apr 29, 2021

How has this one resolved @taureandyernv ?

@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@github-actions
Copy link

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@harrism
Copy link
Member

harrism commented Aug 2, 2022

To summarize, there are two PIP APIs: the non-indexed one which is limited to 31 polygons, and the indexed (quadtree) spatial join, which is limited only to the size representable using the index integer type.

We could update the notebook to use the latter. however as @zhangjianting points out the former is still useful when the overhead of building an index is not desirable.

@github-actions
Copy link

github-actions bot commented Sep 2, 2022

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request Needs Triage Need team to review and classify
Projects
Status: Todo
Development

No branches or pull requests

5 participants