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

Adding Spherical Surface Processing Capability to Point-in-Polygon Queries #1212

Merged
merged 35 commits into from
Aug 3, 2023

Conversation

ayasar70
Copy link
Contributor

@ayasar70 ayasar70 commented Jul 7, 2023

Description

This PR aims to solve point-in-polygon queries using three-dimensional cartesian coordinates on spherical surfaces. Users may prefer to use that approach for more accurate computations.
This PR "closes #1202 "

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@rapids-bot
Copy link

rapids-bot bot commented Jul 7, 2023

Pull requests from external contributors require approval from a rapidsai organization member with write permissions or greater before CI can begin.

@github-actions github-actions bot added the libcuspatial Relates to the cuSpatial C++ library label Jul 7, 2023
@isVoid isVoid added feature request New feature or request non-breaking Non-breaking change labels Jul 11, 2023
cpp/include/cuspatial/geometry/vec_3d.hpp Outdated Show resolved Hide resolved
cpp/include/cuspatial/geometry/vec_3d.hpp Outdated Show resolved Hide resolved
auto p3left = is_left(p1, p2, p3);
auto p4left = is_left(p1, p2, p4);

auto alpha = dot(p1, p3);
Copy link
Contributor

Choose a reason for hiding this comment

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

This says that if any of the end point of the arcs isn't in the same hemisphere, the arcs cannot intersect? Sounds like a very strict test.

Copy link
Member

Choose a reason for hiding this comment

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

Please cite the origin of this algorithm if you can @ayasar70

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was an old paper, and I was not able to find it yet. If I cannot find it, I am going to work on the verification.

@ayasar70 ayasar70 marked this pull request as ready for review July 28, 2023 04:57
@ayasar70 ayasar70 requested a review from a team as a code owner July 28, 2023 04:57
@ayasar70 ayasar70 requested review from trxcllnt and isVoid July 28, 2023 04:57
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Thanks for making this contribution!

TYPED_TEST(PointInPolygonTest, OnePolygonOneRingSpherical)
{
CUSPATIAL_RUN_TEST(this->run_spherical_test,
{{-2503.357, -4660.203, 3551.245},
Copy link
Contributor

Choose a reason for hiding this comment

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

What CRS is this coordinate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are projections that consider the world as a sphere.

Copy link
Contributor

@isVoid isVoid Aug 2, 2023

Choose a reason for hiding this comment

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

Ah I see, I remember you said this is the (X, Y, Z) coordinate using the center of earth as (0, 0, 0). For this algorithm it shouldn't matter how you choose the orthonormal basis as long as all coordinates are consistent. It would be nice to mention this in the docstring.

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @ayasar70 ! This is my initial review. Various doc suggestions and a couple of algorithmic questions/suggestions.

cpp/include/cuspatial/geometry/vec_3d.hpp Outdated Show resolved Hide resolved
cpp/include/cuspatial/geometry/vec_3d.hpp Outdated Show resolved Hide resolved
cpp/include/cuspatial/geometry/vec_3d.hpp Outdated Show resolved Hide resolved
ayasar70 and others added 6 commits July 30, 2023 21:41
Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>
Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>
Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>
Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>
Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>
@harrism
Copy link
Member

harrism commented Aug 2, 2023

/ok to test

@harrism
Copy link
Member

harrism commented Aug 2, 2023

Just realized CI hasn't been running for this PR!

@harrism
Copy link
Member

harrism commented Aug 2, 2023

/ok to test

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

I had one remaining unanswered question (about points on the boundary of the polygon. Please answer it before merging.

I will go ahead and approve. Nice work, and thank you. I note that this PR is just the beginning of support for this feature -- spherical PiP is only implemented in the C++ header-only API. To be a full feature it needs implementation in the column-based API and exposure in the Python API.

@ayasar70
Copy link
Contributor Author

ayasar70 commented Aug 2, 2023

I had one remaining unanswered question (about points on the boundary of the polygon. Please answer it before merging.

I will go ahead and approve. Nice work, and thank you. I note that this PR is just the beginning of support for this feature -- spherical PiP is only implemented in the C++ header-only API. To be a full feature it needs implementation in the column-based API and exposure in the Python API.

Thanks! Sorry, I missed the question before. I have replied. Just talked with @isVoid, after this PR I am going to start working on column-based API and Python API.

@isVoid
Copy link
Contributor

isVoid commented Aug 3, 2023

/ok to test

@isVoid
Copy link
Contributor

isVoid commented Aug 3, 2023

/merge

@rapids-bot rapids-bot bot merged commit 1b1de7c into rapidsai:branch-23.08 Aug 3, 2023
55 checks passed
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 libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[FEA]: Adding Spherical Surface Processing Capability to Point-in-Polygon Queries
3 participants