move_coordinates supports generators

whisk commented 1 year ago

You can now pass generator to move_coordinates like this:

    coords = ((i, i + 1) for i in range(10))
    moved_coords = move_coordinates(coords, (1, 1))

That creates a small problem for mypy, as LineType is a Sequence, not Iterable. So probably to make typing concise we should make LineType an Iterable, or to introduce new type.

pep8speaks commented 1 year ago

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! :beers:

Comment last updated at 2023-11-12 22:08:46 UTC

watermelon-copilot-for-code-review[bot] commented 1 year ago

WatermelonAI Summary

AI Summary deactivated by whisk

GitHub PRs

#188 - move_coordinates supports generators - By whisk Just now
#185 - added force_2d factory and move_coordinates function #180 - By whisk 1 weeks ago
#183 - introduced force_2d for a subset of geometries #180 - By whisk 1 weeks ago

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!

what-the-diff[bot] commented 1 year ago

PR Summary

Change in import statement in pygeoif/functions.py and tests/test_functions.py These changes impact two files. An import statement has been included in the pygeoif/functions.py and tests/test_functions.py to help the subsequent functions to make use of Python's generator function, part of Python's collection library. This allows the functions to efficiently handle large datasets.
Enhancement in Testing for move_coordinates function A number of new tests have been added in the tests/test_functions.py file. New tests check the performance of moving coordinates and ensure that function handles multiple dimensions and generators efficiently. These tests will ensure the precision and robustness of the move_coordinates function.

watermelon-copilot-for-code-review[bot] commented 1 year ago

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

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ff2e39c) 100.00% compared to head (e87144b) 100.00%.

:exclamation: Current head e87144b differs from pull request most recent head 2e4c7c5. Consider uploading reports for the commit 2e4c7c5 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #188 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 21 21 Lines 2412 2430 +18 ========================================= + Hits 2412 2430 +18 ``` | [Files](https://app.codecov.io/gh/cleder/pygeoif/pull/188?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Ledermann) | Coverage Δ | | |---|---|---| | [pygeoif/functions.py](https://app.codecov.io/gh/cleder/pygeoif/pull/188?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Ledermann#diff-cHlnZW9pZi9mdW5jdGlvbnMucHk=) | `100.00% <100.00%> (ø)` | | | [tests/test\_functions.py](https://app.codecov.io/gh/cleder/pygeoif/pull/188?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Christian+Ledermann#diff-dGVzdHMvdGVzdF9mdW5jdGlvbnMucHk=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cleder commented 1 year ago

Thank you for your input, but don't overthink this. Perfect is the opposite of done. While we are able to pass in other types than just CoordinatesType the main use case is to move pygeoif coordinates. Type hints in python are just that, hints. If one wants to pass in generators, be my guest, it will work, although we have to convince mypy that we know what we are doing, but we don't have to cater for all edge-cases.

Remember:

>>> import this
The Zen of Python
...
Beautiful is better than ugly.
Simple is better than complex.
Readability counts.
Special cases aren't special enough to break the rules.
Although practicality beats purity.
Now is better than never.
....

If you want to improve on the functions, try to find edge-cases not covered by tests with mutmut, I think this is a more worthy candidate to invest your time in.

whisk commented 1 year ago

I see you point on mypy.

The thing I wanted to fix is that passing coordinates would't work simply because of this:

TypeError: 'generator' object is not subscriptable

in line if isinstance(coordinates[0], (int, float)):

If passing generators should never be supported, then I must have misunderstood you comment about them.

cleder commented 1 year ago

Sorry, I must admit that my previous comment must have been confusing

cleder / pygeoif