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

introduced force_2d for a subset of geometries #180 #183

Merged
merged 5 commits into from
Nov 5, 2023

Conversation

whisk
Copy link
Contributor

@whisk whisk commented Oct 30, 2023

Added force_2d function for Point, MultiPoint, LineString, Polygon and all descending classes (if any). No support for MultiLineString and MultiPolygon yet.

@pep8speaks
Copy link

pep8speaks commented Oct 30, 2023

Hello @whisk! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-11-05 09:49:32 UTC

@what-the-diff
Copy link

what-the-diff bot commented Oct 30, 2023

PR Summary

  • Introduction of a new function called force_2d
    This update includes a new function named force_2d added to the pygeoif/factories.py file. This function helps handle operations related to 2-dimensional geometries more efficiently in our application.

  • Incorporation of unit tests for newly added force_2d function
    To ensure that this new function performs effectively and error-free, unit tests have been written and included in tests/test_factories.py. These tests will confirm the correctness of the force_2d function by checking it in various scenarios, ensuring robustness of our application.

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (5b461c7) 100.00% compared to head (a213905) 99.83%.

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     
Files Coverage Δ
pygeoif/functions.py 100.00% <100.00%> (ø)
tests/test_factories.py 100.00% <100.00%> (ø)
tests/test_functions.py 100.00% <100.00%> (ø)
pygeoif/factories.py 97.27% <78.94%> (-2.73%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cleder
Copy link
Owner

cleder commented Oct 31, 2023

Hi @whisk, thanks for your contribution.

I think that the force_2d and force_3d could be cleaner to implement by operating on the __geo_interface__ like pygeoif.functions.compare_geo_interface

def force_2d(geometry: Union[GeoType, GeoCollectionType]) -> Union[Geometry, GeometryCollection]:

For force_2d you can zip the coordinates with (0 ,0),

def force_3d(geometry: Union[GeoType, GeoCollectionType], z: float = 0) -> Union[Geometry, GeometryCollection]:

For force_3d you could use zip_longest using z as the fillvalue

To convert the resulting __geo_interface__ dictionary back into a geometry, you may use shape

I hope this makes sense.

@cleder
Copy link
Owner

cleder commented Oct 31, 2023

https://realpython.com/python-zip-function/ and https://ioflood.com/blog/python-zip-function/ seem to be useful to learn more about zip

@cleder
Copy link
Owner

cleder commented Oct 31, 2023

Approach I'd take:

def move_coordinate(coordinate: Sequence[float], move_by: Sequence[float]) -> Tuple[float, ...]:
    if len(coordinate) > len(move_by):
        return tuple(c + m for c, m in zip(coordinate, move_by))
    else:
        return tuple(c + m for c, m in zip_longest(coordinate, move_by, fillvalue=0.0))
>>> move_coordinate((0,0), (0,0,0))
(0, 0, 0.0)
>>> move_coordinate((0,0), (0,0,1))
(0, 0, 1.0)
>>> move_coordinate((0,0,0), (0,0,1))
(0, 0, 1)
>>> move_coordinate((0,0,0), (0,0))
(0, 0)
>>> move_coordinate((1,1,1), (0,0))
(1, 1)
def move_coordinates(coordinates, move_by):
    try:
        return tuple(move_coordinates(c, move_by) for c in coordinates)
    except TypeError:
        return move_coordinate(coordinates, move_by)
>>> move_coordinates((1,0), [0,0,5])
(1, 0, 5.0)
>>> c = ((0,0), (0,1), ((1,0), (1,1), ((2,0) , (2,1), (2,2))))
>>> move_coordinates(c, [0,0,5])
[(0, 0, 5.0), (0, 1, 5.0), [(1, 0, 5.0), (1, 1, 5.0), [(2, 0, 5.0), (2, 1, 5.0), (2, 2, 5.0)]]]
>>> c3d = ((0,0,0), (0,1,1), ((1,0,2), (1,1,3), ((2,0,4) , (2,1,5), (2,2,6))))
>>> move_coordinates(c3d, [0,0])
[(0, 0), (0, 1), [(1, 0), (1, 1), [(2, 0), (2, 1), (2, 2)]]]

This can be used by force_2d with (0, 0) as move_by and force_3d with (0, 0, z) as move_by
What do you think @whisk ?

@whisk
Copy link
Contributor Author

whisk commented Oct 31, 2023

Hi, I refactored force_2d by using __geo_interface__, like functions compare_geo_interface and shape do. This is much simpler and I think that's what you were looking for.

I also added move_coordinate and move_coordinates functions.

>>> move_coordinates(((0, 0), (-1, 1)), (-1, 1, 0))
((-1, 1, 0), (-2, 2, 0))
"""
if isinstance(coordinates, (tuple, list)) and isinstance(
Copy link
Contributor Author

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.

Copy link
Owner

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))

Copy link
Contributor Author

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?

Copy link
Owner

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

Copy link
Owner

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) 👍

Copy link
Contributor Author

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 Summary

AI 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.
🍉🫶
Try us on VSCode!

WatermelonAI Summary

AI 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.
🍉🫶
Try us on VSCode!

WatermelonAI Summary

AI 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.
🍉🫶
Have you starred Watermelon?

WatermelonAI Summary

AI 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.
🍉🫶
Try us on any JetBrains IDE!

WatermelonAI Summary

AI 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.
🍉🫶
Have you starred Watermelon?

WatermelonAI Summary

AI 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.
🍉🫶
Try us on VSCodium!

WatermelonAI Summary

AI 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.
🍉🫶
Try us on VSCodium!

WatermelonAI Summary

AI 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 Summary

AI 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 Summary

AI 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 Summary

AI 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 Summary

AI 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.
🍉🫶
Try us on VSCodium!

WatermelonAI Summary

AI 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.
🍉🫶

Copy link
Owner

@cleder cleder left a 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)]
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
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)]
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
assert list(p2d.geoms) == [geometry.Point(-1, 1), geometry.Point(2, 3)]
assert p2d.geoms)== (geometry.Point(-1, 1), geometry.Point(2, 3))

Comment on lines +194 to +195
coordinate: Sequence[float],
move_by: Sequence[float],
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
coordinate: Sequence[float],
move_by: Sequence[float],
coordinate: PointType,
move_by: PointType,

and add from pygeoif.types import PointType

>>> move_coordinates(((0, 0), (-1, 1)), (-1, 1, 0))
((-1, 1, 0), (-2, 2, 0))
"""
if isinstance(coordinates, (tuple, list)) and isinstance(
Copy link
Owner

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))

Comment on lines +215 to +216
coordinates: Sequence[Any],
move_by: Sequence[float],
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
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]:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
) -> 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)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
raise TypeError(msg)
raise AttributeError(msg)

Copy link
Owner

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())

@cleder cleder merged commit 216525b into cleder:develop Nov 5, 2023
19 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants