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

[DOC] Documented method and code location mismatches #525

Closed
isVoid opened this issue May 12, 2022 · 6 comments
Closed

[DOC] Documented method and code location mismatches #525

isVoid opened this issue May 12, 2022 · 6 comments
Assignees
Labels
doc Documentation

Comments

@isVoid
Copy link
Contributor

isVoid commented May 12, 2022

During #523 , I noticed many methods in cuSpatial are documented in one place, but the actual definition is in another. For example:

Location of incorrect documentation
polyline_bounding_boxes is under "spatial indexing" category.

But in code the methods is in gis.py:

def polyline_bounding_boxes(poly_offsets, xs, ys, expansion_radius):

directed_hausdorff_distance is under trajectories category, but was exposed in gis.py in code:

def directed_hausdorff_distance(xs, ys, space_offsets):

Describe the problems or issues found in the documentation
A blind fix to the issue is that the methods should be in the file named after the same category in documentation. However a priori to the question is that we know the categories actually makes sense. Currently we have spatial indexing, gis and trajectories to sort computing related APIs, internals to document storage foramts, geopandas computatbility and IO to document interops.

The computing related APIs are slightly confusing since gis and trajectories are two slightly overlapped concepts. In the bigger picture the category of computing methods should relate to the problem we try to solve with cuSpatial as a package. I'd like to get some opinions from the community.

Besides the above, geoArrow package currently is placed within cuspatial.geometry module which is eccentric, in two ways. First the name implies geometric operations, but a data structure was defined instead. Second, a well defined data type system is the center piece to a library. Though geoarrow is largely a work in progress definition and cuSpatial will remain it's functional interface for foreseeable future, we should experiment using geoArrow/geodataframe to organize our input, instead of using legacy SoA array format. This is an early thought and I haven't done any verification work yet. The biggest obstacle I can imagine is that the legacy APIs may define the buffer layout differently to geoArrow (MultiLineStrings being an example). Standardizing these inputs may require significant amount of rework of existing APIs.

@isVoid isVoid added doc Documentation Needs Triage Need team to review and classify labels May 12, 2022
rapids-bot bot pushed a commit that referenced this issue Jun 1, 2022
This PR adds doxygen documentation for libcuspatial.

In general, groupings to c++ APIs per our discussion on cuspatial method categories. There are 4 main categories:
- spatial APIs
- trajectory APIs
- spatial indexing
- spatial join

I took liberty on sub-categories and is subject to discussion and change, as part of discussion in #525 .

Many docstring for APIs were missing and are added in this PR. Such as factory methods in `type_utils.hpp`.

Some docstrings broke when being rendered in pages, such as `point_in_polygon`, these were also fixed.

Authors:
  - Michael Wang (https://github.com/isVoid)
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Christopher Harris (https://github.com/cwharris)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #534
@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 Jun 13, 2022

I think we should restructure the library packages as the requirements for cuSpatial are better defined. If you think that it is confusing enough to reduce further adoption, combining trajectories and gis in the simplest way forward.

Besides the above, geoArrow package currently is placed within cuspatial.geometry module which is eccentric, in two ways. First the name implies geometric operations, but a data structure was defined instead. Second, a well defined data type system is the center piece to a library. Though geoarrow is largely a work in progress definition and cuSpatial will remain it's functional interface for foreseeable future, we should experiment using geoArrow/geodataframe to organize our input, instead of using legacy SoA array format. This is an early thought and I haven't done any verification work yet. The biggest obstacle I can imagine is that the legacy APIs may define the buffer layout differently to geoArrow (MultiLineStrings being an example). Standardizing these inputs may require significant amount of rework of existing APIs.

I agree completely that future input should be organized in the geoArrow format, and it should be pretty straight forward.

@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.

@jarmak-nv jarmak-nv moved this to Todo in cuSpatial Aug 2, 2022
rapids-bot bot pushed a commit that referenced this issue Aug 22, 2022
…mplementation in internal `_column` package (#657)

This PR moves geoseries and geodataframe into `core` package and moves `geocolumn`, `geometa` into internal `_column` package as they are considered non-public interface. `pygeoarrow` is moved to `io` package as it's obviously about interoperability.

Minor changes includes removing `geoutils` module and moves the helper function into geodataframe since it's only used in one module.

Contributes to #644 #599 #525

Authors:
  - Michael Wang (https://github.com/isVoid)

Approvers:
  - H. Thomson Comer (https://github.com/thomcom)

URL: #657
rapids-bot bot pushed a commit that referenced this issue Aug 23, 2022
This PR contributes to #644, creates a `spatial` package for spatial related functions. Divided into six sub categories: `measure`, `bound`, `index`, `join`, `project`, `filter`.

Also contributes to #525, #599

Authors:
  - Michael Wang (https://github.com/isVoid)

Approvers:
  - H. Thomson Comer (https://github.com/thomcom)
  - Mark Harris (https://github.com/harrism)

URL: #656
@harrism
Copy link
Member

harrism commented Aug 24, 2022

@isVoid I think you have made progress on this issue with recent PRs. Could you perhaps add a checklist to track what remains?

@harrism harrism removed the Needs Triage Need team to review and classify label Aug 24, 2022
@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.

@isVoid
Copy link
Contributor Author

isVoid commented Sep 26, 2022

This is fixed thanks to recent refactors.

@isVoid isVoid closed this as completed Sep 26, 2022
Repository owner moved this from Todo to Done in cuSpatial Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation
Projects
Status: Done
Development

No branches or pull requests

3 participants