-
Notifications
You must be signed in to change notification settings - Fork 27
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
introduced force_2d for a subset of geometries #180 #183
Conversation
PR Summary
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #183 +/- ##
===========================================
- Coverage 100.00% 99.83% -0.17%
===========================================
Files 21 21
Lines 2299 2434 +135
===========================================
+ Hits 2299 2430 +131
- Misses 0 4 +4
☔ View full report in Codecov by Sentry. |
Hi @whisk, thanks for your contribution. I think that the
For
For To convert the resulting I hope this makes sense. |
https://realpython.com/python-zip-function/ and https://ioflood.com/blog/python-zip-function/ seem to be useful to learn more about |
Approach I'd take:
This can be used by force_2d with |
Hi, I refactored I also added |
pygeoif/functions.py
Outdated
>>> move_coordinates(((0, 0), (-1, 1)), (-1, 1, 0)) | ||
((-1, 1, 0), (-2, 2, 0)) | ||
""" | ||
if isinstance(coordinates, (tuple, list)) and isinstance( |
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.
I preferred to check types explicitly rather what catch TypeError exception.
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.
What happens if I try:
coords = ((i, 2*i, 3*i) for i in range(100))
move_coordinates(coords, (0,0))
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 pointing this out!
As far as I see, you refactored this lines to use CoordinatesType
, not Sequence[Any]
, so this function just can't accept generators anyway.
Not sure if generators still need to be supported in move_coordinates
?
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.
You still CAN pass in a generator, it is just that mypy will complain that you should not do that
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 again for your help, your work will be in the next release (1.2) 👍
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.
You still CAN pass in a generator, it is just that mypy will complain that you should not do that
Yes, yes, you are right. I'm probably just too used to strict typing :)
Please consider PR #188 regarding it.
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
WatermelonAI SummaryAI Summary deactivated by whisk No results found in GitHub PRs :( No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
for more information, see https://pre-commit.ci
WatermelonAI SummaryAI Summary deactivated by whisk GitHub PRs
No results found in Jira Tickets :( No results found in Confluence Docs :( No results found in Slack Threads :( No results found in Notion Pages :( No results found in Linear Tickets :( No results found in Asana Tasks :( pygeoif is an open repo and Watermelon will serve it for free. |
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.
Nice work :-) Thanks a lot!
[geometry.Point(-1, 1, 0), geometry.Point(-2, 2, 0)], | ||
) | ||
gc2d = factories.force_2d(gc) | ||
assert list(gc2d.geoms) == [geometry.Point(-1, 1), geometry.Point(-2, 2)] |
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.
assert list(gc2d.geoms) == [geometry.Point(-1, 1), geometry.Point(-2, 2)] | |
assert gc2d.geoms == (geometry.Point(-1, 1), geometry.Point(-2, 2)) |
# 2d to 2d (no actual change) | ||
p = geometry.MultiPoint([(-1, 1), (2, 3)]) | ||
p2d = factories.force_2d(p) | ||
assert list(p2d.geoms) == [geometry.Point(-1, 1), geometry.Point(2, 3)] |
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.
assert list(p2d.geoms) == [geometry.Point(-1, 1), geometry.Point(2, 3)] | |
assert p2d.geoms)== (geometry.Point(-1, 1), geometry.Point(2, 3)) |
coordinate: Sequence[float], | ||
move_by: Sequence[float], |
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.
coordinate: Sequence[float], | |
move_by: Sequence[float], | |
coordinate: PointType, | |
move_by: PointType, |
and add from pygeoif.types import PointType
pygeoif/functions.py
Outdated
>>> move_coordinates(((0, 0), (-1, 1)), (-1, 1, 0)) | ||
((-1, 1, 0), (-2, 2, 0)) | ||
""" | ||
if isinstance(coordinates, (tuple, list)) and isinstance( |
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.
What happens if I try:
coords = ((i, 2*i, 3*i) for i in range(100))
move_coordinates(coords, (0,0))
coordinates: Sequence[Any], | ||
move_by: Sequence[float], |
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.
coordinates: Sequence[Any], | |
move_by: Sequence[float], | |
coordinates: Union[CoordinatesType, MultiCoordinatesType], | |
move_by: PointType, |
coordinates: Sequence[Any], | ||
move_by: Sequence[float], | ||
z: float = 0, | ||
) -> Sequence[Any]: |
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.
) -> Sequence[Any]: | |
) -> Union[CoordinatesType, MultiCoordinatesType]: |
geometry = context if isinstance(context, dict) else mapping(context) | ||
if not geometry: | ||
msg = "Object does not implement __geo_interface__" | ||
raise TypeError(msg) |
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.
raise TypeError(msg) | |
raise AttributeError(msg) |
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.
What happens if I call
force_2d(object())
Added
force_2d
function for Point, MultiPoint, LineString, Polygon and all descending classes (if any). No support for MultiLineString and MultiPolygon yet.