-
Notifications
You must be signed in to change notification settings - Fork 152
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
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Thanks @jarmak-nv for the quick review. Made the changes. |
There was a problem hiding this 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.
There was a problem hiding this 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
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
There was a problem hiding this 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
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
This can't be merged into 23.08. Please retarget. |
We were hoping to slip it in with the other hotfixes. There is another PR for cuspatial that will be part of it. May we still keep it in 23.08?
From: Mark Harris ***@***.***>
Sent: Monday, August 14, 2023 2:39 PM
To: rapidsai/cuspatial ***@***.***>
Cc: Taurean Dyer ***@***.***>; Author ***@***.***>
Subject: Re: [rapidsai/cuspatial] [REVIEW] Added cudf example code for haversine distance (PR #1259)
This can't be merged into 23.08. Please retarget.
—
Reply to this email directly, view it on GitHub<#1259 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ALGCYZBG23UOZNKTF4FMVYDXVKLFVANCNFSM6AAAAAA3QABYNY>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
superseded by #1260 |
There was a problem hiding this 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.
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 The blog post below outlines how you can do this: |
/ok to test |
Thanks for the review, @harrism ! I resolved all the changes requested and it's ready for rerun. |
reclosing in favor of #1260 |
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