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

Python API for point-linestring nearest points #681

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Sep 20, 2022

Description

This PR adds python API for point-linestring nearest points. Closes #646 .
Depend on #658 #686

This PR also moves Feature_Enum from io module to geometa module to make sure the coding is consistent and reused throughout the codebase.

Checklist

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

isVoid and others added 30 commits June 14, 2022 10:52
…into feature/header_only_point_linestring_distance
Co-authored-by: Mark Harris <mharris@nvidia.com>
…b.com:isVoid/cuspatial into feature/header_only_point_linestring_distance
…into feature/header_only_point_linestring_distance
@github-actions github-actions bot removed the libcuspatial Relates to the cuSpatial C++ library label Sep 22, 2022
@isVoid isVoid requested review from thomcom, harrism and trxcllnt and removed request for vyasr and jrhemstad September 22, 2022 22:00
python/cuspatial/cuspatial/_lib/nearest_points.pyx Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/_lib/nearest_points.pyx Outdated Show resolved Hide resolved
check_dtype=False,
check_index_type=False,
check_less_precise=True,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

You have many failure cases in the python code that you aren't testing for here, like https://github.com/rapidsai/cuspatial/pull/681/files#diff-6e59ef32765771de56798ec48c85a59ba2bb65f4c10f6e06326a59ad82a793ffR42, also lines 56, 59, and 64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added all failure case tests except line 64 because I don't think notimplementederror should be tested (as it will be removed). Let me know if you insist.

@thomcom
Copy link
Contributor

thomcom commented Sep 23, 2022

Looks really good. I think you need some more failure cases in python and a noticed a couple of variable renamings.

@isVoid isVoid requested a review from thomcom September 26, 2022 20:09
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.

Just one doc clarification. Otherwise approve. Great work!

@isVoid
Copy link
Contributor Author

isVoid commented Sep 28, 2022

@gpucibot merge

@harrism
Copy link
Member

harrism commented Sep 28, 2022

@isVoid you still need approval from @thomcom

@isVoid
Copy link
Contributor Author

isVoid commented Sep 28, 2022

For some reason I thought I had it... welp, no rush then.

@rapids-bot rapids-bot bot merged commit 5de8212 into rapidsai:branch-22.10 Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Related to CMake code or build configuration feature request New feature or request non-breaking Non-breaking change Python Related to Python code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEA] Nearest point and segment index of linestring to a point (pairwise)
3 participants