-
Notifications
You must be signed in to change notification settings - Fork 8
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
ENH: Added length and perimeter implementation #77
Conversation
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!
What's broken about the perimeter?
I am still getting used to thinking in spherical geometries. The test case I wrote for the perimeter function used a polygon that was rectangular in my head, so I expected its perimeter to be Sorry for the confusion, everything was fine from the start! |
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.
Looks good!
Do you just want to add it to api.rst as well?
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 @JoelJaeschke!
One question about the API: should we follow BigQuery's API or shapely's API? Or a mix of both? Some options:
- Expose separate
length
andperimeter
functions, with value zero returned for "incompatible" geography (like BigQuery, currently implemented in this PR) - Expose a unique
length
function (like Shapely) - Expose
length
andperimeter
functions wherelength
also returns the perimeter for an input Polygon.
Any thoughts on this @jorisvandenbossche? |
I think I like option 3: we provide a (we should maybe consider adding a I am also fine with leaving that for a follow-up, though, either way is fine. |
I would be happy to change the PR accordingly if option 3 is the way forward! |
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.
Let's choose the easy way and leave length
and perimeter
as it is in this PR for now.
Thanks @JoelJaeschke !
No description provided.