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

Shapely 2.0 compatibility #214

Merged
merged 3 commits into from
Oct 25, 2023
Merged

Shapely 2.0 compatibility #214

merged 3 commits into from
Oct 25, 2023

Conversation

knthls
Copy link

@knthls knthls commented May 3, 2023

I have made some modifications to replace PyGEOS with shapely 2.0

PyGEOS was merged with shapely, which is why shapely offers exactly the same API as PyGEOS. I have basically renamed all imports from pygeos to imports from shapely and I have eliminated the additional parts that deal with shapely/pygeos compatibility. My modifications address issues #213 and #208.
I have tested my modifications an verified that they work, given that the environment variable USE_PYGEOS is set to 0.

Please Note that my modifications introduce a hard requirement for shapely 2.0 so it may not be ready to merge, depending on your policy regarding support of older libraries. I still hope that this contribution saves you some time.

@knthls
Copy link
Author

knthls commented May 9, 2023

Please let me know if I can do anything to help this getting merged. I guess the CI pipeline is broken.

python: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by /__t/Python/3.10.11/x64/lib/libpython3.10.so.1.0)
Error: Process completed with exit code 1.

I have applied black and flake8, but I think that it actually ignores the cython stuff.

@HTenkanen
Copy link
Collaborator

@knthls Thanks a lot for helping out!! 🙂 I've been frustratingly busy with my day job, and getting back to pyrosm seem to continuesly get pushed further on my todo-list. 🤷🏻‍♂️ Hence, all the help with maintaining and developing this further are highly appreciated! 🙏🏻

Indeed it seems that we need to update the tests.yaml workflow which is not up-to-date anymore. I'm quite busy the coming days, so don't have time to fix this until sometime next week, but in case you want to take a look at the CI file, I'd be happy to merge a PR that fixes the pipeline, and after that we can take a closer look at this PR.

@hbruch
Copy link

hbruch commented May 24, 2023

Thanks @knthls for your work! To get the CI fixed, #215 should solve the black/linting issues.
Running USE_PYGEOS=0 pytest -v on my Mac, I still had some test issues (some of them might be related to me not using conda, i.e. I have install issues with pandana/tables):

FAILED tests/test_graph_exports.py::test_igraph_export_by_driving - assert 51420 == 44296
FAILED tests/test_graph_exports.py::test_pdgraph_connectivity - ImportError: 'pandana' needs to be installed in order to export the network for it.
FAILED tests/test_graph_exports.py::test_to_graph_api - ModuleNotFoundError: No module named 'pandana'
FAILED tests/test_network_parsing.py::test_saving_network_to_shapefile - AssertionError: assert [None, None, ...ne, None, ...] == [nan, nan, na...nan, nan, ...]
FAILED tests/test_utils.py::test_timestamp_integer - TypeError: Cannot localize tz-aware Timestamp, use tz_convert for conversions
FAILED tests/test_utils.py::test_OSH_file_without_timestamp - TypeError: Cannot localize tz-aware Timestamp, use tz_convert for conversions
====================================================================================== 6 failed, 99 passed, 2 skipped, 37 warnings in 127.89s (0:02:07) ==

The tz-aware Timestamp issue could be solved by fixing unix_time_to_datetime in utils/__init__.py:

def unix_time_to_datetime(unix_time):
    return pd.Timestamp.utcfromtimestamp(unix_time)

Regarding test_igraph_export_by_driving I wonder, if the used dataset is a fixed dataset or a live dataset which surely will change from time to time.

The pandanas errors are due to pandana install issues, probably only on my side.

@knthls
Copy link
Author

knthls commented May 24, 2023

Thanks @hbruch. I didn't get into this, since I am really busy too.
The only issue that I could confirm is this one.

FAILED tests/test_graph_exports.py::test_igraph_export_by_driving - assert 51420 == 44296

I will try to get into this as soon as possible. It could really could be that I introduced some errors without noticing, since the only thing that I really changes were the imports.

@hbruch
Copy link

hbruch commented May 24, 2023

I don't think, these are introduced errors. The osm dataset for test_igraph_export_by_driving is not static and hence no assertions on exact number of features should be expected (or instead one of the static Helsinki datasets be used). @HTenkanen wsa there a specific reason, why ulanbator is used instead?

@tfardet
Copy link

tfardet commented Aug 11, 2023

I've been using this branch to make sure it worked and, as mentioned by @hbruch, the errors seem entirely unrelated to that PR.
Would it be possible to merge it?

Copy link
Collaborator

@HTenkanen HTenkanen left a comment

Choose a reason for hiding this comment

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

A big thanks for these 🙂

@HTenkanen HTenkanen merged commit 7b1f225 into pyrosm:master Oct 25, 2023
@HTenkanen
Copy link
Collaborator

The test errors are not related to this PR, so will fix them separately:

Thanks @knthls for your work! To get the CI fixed, #215 should solve the black/linting issues. Running USE_PYGEOS=0 pytest -v on my Mac, I still had some test issues (some of them might be related to me not using conda, i.e. I have install issues with pandana/tables):

FAILED tests/test_graph_exports.py::test_igraph_export_by_driving - assert 51420 == 44296
FAILED tests/test_graph_exports.py::test_pdgraph_connectivity - ImportError: 'pandana' needs to be installed in order to export the network for it.
FAILED tests/test_graph_exports.py::test_to_graph_api - ModuleNotFoundError: No module named 'pandana'
FAILED tests/test_network_parsing.py::test_saving_network_to_shapefile - AssertionError: assert [None, None, ...ne, None, ...] == [nan, nan, na...nan, nan, ...]
FAILED tests/test_utils.py::test_timestamp_integer - TypeError: Cannot localize tz-aware Timestamp, use tz_convert for conversions
FAILED tests/test_utils.py::test_OSH_file_without_timestamp - TypeError: Cannot localize tz-aware Timestamp, use tz_convert for conversions
====================================================================================== 6 failed, 99 passed, 2 skipped, 37 warnings in 127.89s (0:02:07) ==

The tz-aware Timestamp issue could be solved by fixing unix_time_to_datetime in utils/__init__.py:

def unix_time_to_datetime(unix_time):
    return pd.Timestamp.utcfromtimestamp(unix_time)

Regarding test_igraph_export_by_driving I wonder, if the used dataset is a fixed dataset or a live dataset which surely will change from time to time.

The pandanas errors are due to pandana install issues, probably only on my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants