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

Add libcuspatial C++ developer guide. #606

Merged
merged 13 commits into from
Aug 29, 2022

Conversation

harrism
Copy link
Member

@harrism harrism commented Jul 28, 2022

Contributes to #598.

Adds a new libcuspatial C++ developer guide.

@harrism harrism requested a review from a team as a code owner July 28, 2022 02:13
@github-actions github-actions bot added the libcuspatial Relates to the cuSpatial C++ library label Jul 28, 2022
@harrism harrism added doc Documentation non-breaking Non-breaking change labels Jul 28, 2022
@harrism harrism self-assigned this Jul 28, 2022
@harrism harrism requested a review from isVoid July 28, 2022 02:15
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!

@harrism
Copy link
Member Author

harrism commented Aug 24, 2022

rerun tests

@harrism harrism added this to the Developer Documentation milestone Aug 24, 2022
Copy link
Contributor

@thomcom thomcom left a comment

Choose a reason for hiding this comment

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

Just a few notes and recommendations. Looks good! Also, seems like a couple of .hpp files got accidentally included.

All input and output iterators must be device-accessible with random access. They must satisfy the
requirements of C++ [LegacyRandomAccessIterator][LinkLRAI]. Output iterators must be mutable.

## Multiple Return Values
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this section go into the libcudf API section?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is useful for both APIs. Perhaps I should reiterate that the header-only API should return via output iterator where possible. However when the API needs to allocate the results, it must use return(s).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is confusing for me because you have grouped the libcudf and header only ## Multiple Return Values together, and you also seem to switch between the two frameworks while you talk about handling multiple return values. As a developer who is either interested in libcudf or interested in contributing to header only, I'd like a little more predictability about which paragraph is for me. This is a small detail though, mostly the document reads well, and I'm not asking you to change anything more.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback. Let mesee what I can do.

Copy link
Member Author

Choose a reason for hiding this comment

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

As a developer who is either interested in libcudf or interested in contributing to header only, I'd like a little more predictability about which paragraph is for me.

For what it's worth, I expect anyone contributing to libcuspatial will likely be contributing to both.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I found that I could clean this up by using parallel structure between the column-based and header-only API sections. I hope this makes it clearer for the reader.

All input and output iterators must be device-accessible with random access. They must satisfy the
requirements of C++ [LegacyRandomAccessIterator][LinkLRAI]. Output iterators must be mutable.

## Multiple Return Values
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is confusing for me because you have grouped the libcudf and header only ## Multiple Return Values together, and you also seem to switch between the two frameworks while you talk about handling multiple return values. As a developer who is either interested in libcudf or interested in contributing to header only, I'd like a little more predictability about which paragraph is for me. This is a small detail though, mostly the document reads well, and I'm not asking you to change anything more.

@harrism
Copy link
Member Author

harrism commented Aug 25, 2022

rerun tests

@harrism
Copy link
Member Author

harrism commented Aug 28, 2022

@gpucibot merge

@harrism
Copy link
Member Author

harrism commented Aug 29, 2022

rerun tests

@rapids-bot rapids-bot bot merged commit cd0f2b3 into rapidsai:branch-22.10 Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants