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

[REVIEW] Added cudf example code for haversine distance #1259

Closed
wants to merge 12 commits into from

Conversation

taureandyernv
Copy link
Contributor

@taureandyernv taureandyernv commented Aug 14, 2023

Created a cudf -> cuspatial example for haversine distance in the docs
Added example code to the api docs with some explanation on benefits of this method

Description

Checklist

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

@taureandyernv taureandyernv requested a review from a team as a code owner August 14, 2023 17:33
@taureandyernv taureandyernv requested review from thomcom and harrism and removed request for a team August 14, 2023 17:33
@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 Aug 14, 2023
@taureandyernv
Copy link
Contributor Author

Thanks @jarmak-nv for the quick review. Made the changes.

Copy link
Contributor

@jarmak-nv jarmak-nv 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 quick updates, LGTM pending CI pass.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Taurean! 🙏

Looks like the style check caught some extraneous spaces

Added suggestions below to drop these

python/cuspatial/cuspatial/core/spatial/distance.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/spatial/distance.py Outdated Show resolved Hide resolved
taureandyernv and others added 2 commits August 14, 2023 13:10
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
@jakirkham jakirkham added doc Documentation non-breaking Non-breaking change labels Aug 14, 2023
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Taurean! 🙏

Looks like the style check caught some line length issues. Added some suggestions to wrap them

Hopefully the last issue here

python/cuspatial/cuspatial/core/spatial/distance.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/spatial/distance.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/spatial/distance.py Outdated Show resolved Hide resolved
taureandyernv and others added 3 commits August 14, 2023 13:21
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
@harrism
Copy link
Member

harrism commented Aug 14, 2023

This can't be merged into 23.08. Please retarget.

@harrism harrism changed the base branch from branch-23.08 to branch-23.10 August 14, 2023 21:38
@taureandyernv
Copy link
Contributor Author

taureandyernv commented Aug 14, 2023 via email

@taureandyernv
Copy link
Contributor Author

superseded by #1260

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 some Python naming suggestions.

python/cuspatial/cuspatial/core/spatial/distance.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/spatial/distance.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/spatial/distance.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/spatial/distance.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/spatial/distance.py Outdated Show resolved Hide resolved
@copy-pr-bot
Copy link

copy-pr-bot bot commented Aug 28, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ajschmidt8
Copy link
Member

This PR has been marked untrusted due to the changes outlined in #1262.

It looks like the PR has unsigned commits.

You can rebase your branch to sign the unsigned commits to avoid having to use an /ok to test comment for every subsequent push.

The blog post below outlines how you can do this:

@harrism
Copy link
Member

harrism commented Aug 28, 2023

/ok to test

@taureandyernv
Copy link
Contributor Author

Thanks for the review, @harrism ! I resolved all the changes requested and it's ready for rerun.
/ok to test

@taureandyernv
Copy link
Contributor Author

reclosing in favor of #1260

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation non-breaking Non-breaking change Python Related to Python code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants