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

cuSpatial Python User Guide #666

Merged
merged 54 commits into from
Sep 30, 2022
Merged

Conversation

thomcom
Copy link
Contributor

@thomcom thomcom commented Sep 6, 2022

This PR adds docs/source/user_guide/index.ipynb, a comprehensive description of the APIs available in cuspatial python and example code for each.

The APIs are separated in a table of contents by the logical structure we've settled on for their location in the API. The demonstrate uses the cuspatial. version of every API, none are accessed using their full python package address.

At the bottom I'm working on some examples of performing contains and within, DE-9IM operations that can be performed easily with point-in-polygon. Otherwise the document is complete.

Depends on #685 and #680

This contributes to or closes #599 by providing an example use case of all python APIs.

@thomcom thomcom mentioned this pull request Sep 6, 2022
@thomcom
Copy link
Contributor Author

thomcom commented Sep 19, 2022

This PR is nearly complete with an incoming .ipynb. I ran into a fairly serious error however that prevents slicing GeoDataframes that contain a GeoSeries, #676, which I'm working on in parallel before this PR can be completed.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added the Python Related to Python code label Sep 21, 2022
@isVoid isVoid requested a review from jarmak-nv September 27, 2022 01:06
@isVoid isVoid added this to the Developer Documentation milestone Sep 27, 2022
@isVoid
Copy link
Contributor

isVoid commented Sep 27, 2022

This looks all very promising and nice to read, I just left some comments mostly about the introduction section on the terms and phrases that I got confused about. The code section is mostly good.

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.

This is a great. I have a number of edits / suggestions. GitHub started doing weird things with my suggestions towards the end (treating the original text as an encoded image).

My biggest concern is that this document is already getting quite long, and as cuSpatial's API grows it is going to get unwieldy. I think going forward we should explore how we can modularize it, and where possible, associate the interactive examples with the individual API doc sections/pages.

docs/source/developer_guide/internals.md Outdated Show resolved Hide resolved
docs/source/developer_guide/internals.md Outdated Show resolved Hide resolved
docs/source/developer_guide/internals.md Outdated Show resolved Hide resolved
docs/source/developer_guide/internals.md Outdated Show resolved Hide resolved
docs/source/developer_guide/internals.md Outdated Show resolved Hide resolved
docs/source/user_guide/users.ipynb Outdated Show resolved Hide resolved
docs/source/user_guide/users.ipynb Outdated Show resolved Hide resolved
docs/source/user_guide/users.ipynb Outdated Show resolved Hide resolved
docs/source/user_guide/users.ipynb Outdated Show resolved Hide resolved
docs/source/user_guide/users.ipynb Outdated Show resolved Hide resolved
@thomcom
Copy link
Contributor Author

thomcom commented Sep 27, 2022

I've addressed the existing reviews.

@thomcom thomcom requested review from harrism and isVoid September 27, 2022 19:33
"## GPU accelerated memory layout\n",
"\n",
"cuspatial uses `GeoArrow` buffers, a GPU-friendly data format for geometric data that is well \n",
"suited for massively parallel programming. See [I/O](#io) on the fastest methods to get your \n",
Copy link
Member

Choose a reason for hiding this comment

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

Test that this link works.

Suggested change
"suited for massively parallel programming. See [I/O](#io) on the fastest methods to get your \n",
"suited for massively parallel programming. See [I/O](#input-output) on the fastest methods to get your \n",

@rapidsai rapidsai deleted a comment from thomcom Sep 27, 2022
docs/source/user_guide/users.ipynb Outdated Show resolved Hide resolved
docs/source/user_guide/users.ipynb Outdated Show resolved Hide resolved
docs/source/user_guide/users.ipynb Outdated Show resolved Hide resolved
docs/source/user_guide/users.ipynb Outdated Show resolved Hide resolved
docs/source/user_guide/users.ipynb Outdated Show resolved Hide resolved
docs/source/user_guide/users.ipynb Outdated Show resolved Hide resolved
@harrism harrism changed the title Cuspatial User Guide cuSpatial Python User Guide Sep 28, 2022
@@ -0,0 +1,1602 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

a Tuple of GeoArrow buffers

But... they are just plain cudf series and dataframes!

I would propose we implement `GeoSeries.from_filewith this function.


Reply via ReviewNB

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 think I'd like to update read_polygon_shapefile to return a GeoSeries, as from_file implies many more file types to support. We could write from_file and check if it is a shapefile, then use our internal reader otherwise use GeoPandas, but performance would be poor for all other file types. What do you think? #715

@harrism
Copy link
Member

harrism commented Sep 28, 2022

@thomcom I propose removing from this PR the speculative sections at the end on building up the rest of DE9IM (contains, etc) using existing primitives. That could be saved for a followup PR.

@thomcom
Copy link
Contributor Author

thomcom commented Sep 29, 2022

I'll remove the stuff at the bottom and then merge.

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.

I have these comments still open:
#666 (comment)
#666 (comment)
#666 (comment)

@thomcom
Copy link
Contributor Author

thomcom commented Sep 30, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0becc88 into rapidsai:branch-22.10 Sep 30, 2022
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 non-breaking Non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[DOC] Improve Python developer documentation
3 participants