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

compatibility with shapely v2? #2076

Closed
mathause opened this issue Sep 2, 2022 · 16 comments
Closed

compatibility with shapely v2? #2076

mathause opened this issue Sep 2, 2022 · 16 comments
Milestone

Comments

@mathause
Copy link
Contributor

mathause commented Sep 2, 2022

Description

The pre-release version of shapely v2 is out & that seems to break cartopy.

Code to reproduce

Install shapely v2 & cartopy and try to import cartopy:

mamba create -n cartopy_shapely_v2 -c conda-forge/label/shapely_dev shapely cartopy
mamba activate cartopy_shapely_v2
python -c "import cartopy"

Traceback

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/mathause/.conda/envs/cartopy_shapely_v2/lib/python3.10/site-packages/cartopy/__init__.py", line 107, in <module>
    import cartopy.crs
  File "/home/mathause/.conda/envs/cartopy_shapely_v2/lib/python3.10/site-packages/cartopy/crs.py", line 27, in <module>
    import cartopy.trace
  File "lib/cartopy/trace.pyx", line 77, in init cartopy.trace
ImportError: cannot import name lgeos
Full environment definition

Operating system

linux

Cartopy version

cartopy 0.20.3

conda list

_libgcc_mutex             0.1                 conda_forge    conda-forge
_openmp_mutex             4.5                       2_gnu    conda-forge
brotli                    1.0.9                h166bdaf_7    conda-forge
brotli-bin                1.0.9                h166bdaf_7    conda-forge
bzip2                     1.0.8                h7f98852_4    conda-forge
c-ares                    1.18.1               h7f98852_0    conda-forge
ca-certificates           2022.6.15            ha878542_0    conda-forge
cartopy                   0.20.3          py310he7eef42_2    conda-forge
certifi                   2022.6.15          pyhd8ed1ab_1    conda-forge
cycler                    0.11.0             pyhd8ed1ab_0    conda-forge
fonttools                 4.37.1          py310h5764c6d_0    conda-forge
freetype                  2.12.1               hca18f0e_0    conda-forge
geos                      3.11.0               h27087fc_0    conda-forge
jpeg                      9e                   h166bdaf_2    conda-forge
keyutils                  1.6.1                h166bdaf_0    conda-forge
kiwisolver                1.4.4           py310hbf28c38_0    conda-forge
krb5                      1.19.3               h08a2579_0    conda-forge
lcms2                     2.12                 hddcbb42_0    conda-forge
ld_impl_linux-64          2.36.1               hea4e1c9_2    conda-forge
lerc                      4.0.0                h27087fc_0    conda-forge
libblas                   3.9.0           16_linux64_openblas    conda-forge
libbrotlicommon           1.0.9                h166bdaf_7    conda-forge
libbrotlidec              1.0.9                h166bdaf_7    conda-forge
libbrotlienc              1.0.9                h166bdaf_7    conda-forge
libcblas                  3.9.0           16_linux64_openblas    conda-forge
libcurl                   7.83.1               h2283fc2_0    conda-forge
libdeflate                1.13                 h166bdaf_0    conda-forge
libedit                   3.1.20191231         he28a2e2_2    conda-forge
libev                     4.33                 h516909a_1    conda-forge
libffi                    3.4.2                h7f98852_5    conda-forge
libgcc-ng                 12.1.0              h8d9b700_16    conda-forge
libgfortran-ng            12.1.0              h69a702a_16    conda-forge
libgfortran5              12.1.0              hdcd56e2_16    conda-forge
libgomp                   12.1.0              h8d9b700_16    conda-forge
liblapack                 3.9.0           16_linux64_openblas    conda-forge
libnghttp2                1.47.0               hff17c54_1    conda-forge
libnsl                    2.0.0                h7f98852_0    conda-forge
libopenblas               0.3.21          pthreads_h78a6416_3    conda-forge
libpng                    1.6.37               h753d276_4    conda-forge
libsqlite                 3.39.2               h753d276_1    conda-forge
libssh2                   1.10.0               hf14f497_3    conda-forge
libstdcxx-ng              12.1.0              ha89aaad_16    conda-forge
libtiff                   4.4.0                h0e0dad5_3    conda-forge
libuuid                   2.32.1            h7f98852_1000    conda-forge
libwebp-base              1.2.4                h166bdaf_0    conda-forge
libxcb                    1.13              h7f98852_1004    conda-forge
libzlib                   1.2.12               h166bdaf_2    conda-forge
matplotlib-base           3.5.3           py310h8d5ebf3_2    conda-forge
munkres                   1.1.4              pyh9f0ad1d_0    conda-forge
ncurses                   6.3                  h27087fc_1    conda-forge
numpy                     1.23.2          py310h53a5b5f_0    conda-forge
openjpeg                  2.5.0                h7d73246_1    conda-forge
openssl                   3.0.5                h166bdaf_1    conda-forge
packaging                 21.3               pyhd8ed1ab_0    conda-forge
pillow                    9.2.0           py310hbd86126_2    conda-forge
pip                       22.2.2             pyhd8ed1ab_0    conda-forge
proj                      9.0.1                h93bde94_1    conda-forge
pthread-stubs             0.4               h36c2ea0_1001    conda-forge
pyparsing                 3.0.9              pyhd8ed1ab_0    conda-forge
pyproj                    3.3.1           py310hf94497c_1    conda-forge
pyshp                     2.3.1              pyhd8ed1ab_0    conda-forge
python                    3.10.6          ha86cf86_0_cpython    conda-forge
python-dateutil           2.8.2              pyhd8ed1ab_0    conda-forge
python_abi                3.10                    2_cp310    conda-forge
readline                  8.1.2                h0f457ee_0    conda-forge
scipy                     1.9.1           py310hdfbd76f_0    conda-forge
setuptools                65.3.0             pyhd8ed1ab_1    conda-forge
shapely                   2.0a1           py310h721e2b3_0    conda-forge/label/shapely_dev
six                       1.16.0             pyh6c4a22f_0    conda-forge
sqlite                    3.39.2               h4ff8645_1    conda-forge
tk                        8.6.12               h27826a3_0    conda-forge
tzdata                    2022c                h191b570_0    conda-forge
unicodedata2              14.0.0          py310h5764c6d_1    conda-forge
wheel                     0.37.1             pyhd8ed1ab_0    conda-forge
xorg-libxau               1.0.9                h7f98852_0    conda-forge
xorg-libxdmcp             1.1.3                h7f98852_0    conda-forge
xz                        5.2.6                h166bdaf_0    conda-forge
zstd                      1.5.2                h6239696_4    conda-forge

pip list

@greglucas
Copy link
Contributor

Thanks for testing that! Looks like lgeos has been removed upstream: shapely/shapely#1163

Maybe it is time to look more at removing GEOS from our dependencies and fully relying on shapely. I think @QuLogic had a branch working on removing GEOS, but maybe it was too slow before? Perhaps shapely2.0 provides the speedups needed for the switch.

@QuLogic
Copy link
Member

QuLogic commented Sep 3, 2022

I do have a branch for it, and I just rebased it, but haven't looked at the current speed, or whether 2.0 will help with that. There were definitely some optimizations available in pygeos (which I didn't apply yet) that I think was intended to become shapely 2.0, so they might be some improvements available.

@greglucas
Copy link
Contributor

@QuLogic, I just looked through your branches, but didn't see anything pushed recently. Which branch was it?

@dopplershift
Copy link
Contributor

We should definitely make sure we pin 0.21 to Shapely < 2.

@jorisvandenbossche
Copy link
Contributor

I quickly looked into how cartopy is using shapely.geos.lgeos, and that actually doesn't seem necessary (it's only used to get a GEOS context handle, but since you are already interfacing the GEOS C API in cython, cartopy can easily create this context themselves).
I opened a PR to remove that usage of lgeos -> #2080

Regarding compatibility with Shapely 2.0, I think there are two aspects: 1) just run with Shapely 2.0, and 2) make use of new features in Shapely 2.0 so cartopy can stop linking to GEOS itself (and just rely on Shapely to do that), i.e. to solve #805 (and what @QuLogic's branch is attempting).
While the second aspect would certainly be great long-term to avoid installation issues, I think on the short term it would also be nice that cartopy could still just work with Shapely 2.0, so that it is easier for users to test / move to Shapely 2.0 (and so that you can remove the <2 pin).

To get cartopy compatible with Shapely 2.0, a lot of the necessary changes were already done before (#1914, all things we could warn about in advance), and #2080 removes the last bit of API usage that was removed or changed in Shapely 2.0 (except for the usage of geom_factory, which is not easy to emulate in cartopy, so I think we could add a (deprecated) shim for that back to shapely 2.0 for compatibility on the short term: shapely/shapely#1534).
In addition, when running the cartopy test suite, I also discovered that cartopy relies on weakrefs to geometry objects, and it seems that Shapely 2.0 broke that (so in that sense it was actually really useful to get cartopy to "work" with shapely 2.0, so we can discover such issues).

@jorisvandenbossche
Copy link
Contributor

In addition, when running the cartopy test suite, I also discovered that cartopy relies on weakrefs to geometry objects, and it seems that Shapely 2.0 broke that

I opened a fix on the Shapely side (shapely/shapely#1535), although I am also wondering to what extent this is important for cartopy? (I didn't check the exact use case here)


I do have a branch for it, and I just rebased it, but haven't looked at the current speed, or whether 2.0 will help with that. There were definitely some optimizations available in pygeos (which I didn't apply yet) that I think was intended to become shapely 2.0, so they might be some improvements available.

@QuLogic this is this branch: https://github.com/QuLogic/cartopy/tree/shapely? (based on your comment from #805 (comment)) It might be easier to further discuss this when opening a draft PR?

@greglucas
Copy link
Contributor

In addition, when running the cartopy test suite, I also discovered that cartopy relies on weakrefs to geometry objects, and it seems that Shapely 2.0 broke that

I opened a fix on the Shapely side (shapely/shapely#1535), although I am also wondering to what extent this is important for cartopy? (I didn't check the exact use case here)

With geomtries being hashable in 2.0 the weakrefs could potentially be removed from Cartopy because it looks like we are using them in place of the old non-hashable geometries according to this comment:

# As Shapely geometries cannot be relied upon to be
# hashable, we have to use a WeakValueDictionary to manage
# their weak references. The key can then be a simple,
# "disposable", hashable geom-key object that just uses the
# id() of a geometry to determine equality and hash value.
# The only persistent, strong reference to the geom-key is
# in the WeakValueDictionary, so when the geometry is
# garbage collected so is the geom-key.
# The geom-key is also used to access the WeakKeyDictionary
# cache of transformed geometries. So when the geom-key is
# garbage collected so are the transformed geometries.
geom_key = _GeomKey(geom)
FeatureArtist._geom_key_to_geometry_cache.setdefault(
geom_key, geom)
mapping = FeatureArtist._geom_key_to_path_cache.setdefault(
geom_key, {})

@greglucas
Copy link
Contributor

Slowdown: I get ~2x slowdown from @QuLogic's branch removing GEOS, which I think agrees with what he found before. (Rebased branch: https://github.com/greglucas/cartopy/tree/qulogic-delete-proj-geos) Some profiling tests (project_linear from the benchmarks) show some rough areas where we could improve.

With GEOS

Screenshot from 2022-09-13 19-48-40

Without GEOS

Screenshot from 2022-09-13 19-46-48

Some interesting observations from above:

  1. We are wasting a lot of time in pyproj's utils._copytobuffer. This is because we are thrashing the transform method calling it 1M times, rather than calling it one time with 1M points. I had a branch playing with this a bit, but haven't found the time to really investigate it in-depth. @snowman2 maybe you have some ideas for how to avoid the _copytobuffer calls?
  2. The most time spent in Shapely seems to be in getting coordinates, creating points and linestrings, not necessarily the covers() operation. (Most of the methods to the right of the figure are Shapely methods, which is where the additional time comes from relative to figure 1 with GEOS)

@snowman2
Copy link
Contributor

@jorisvandenbossche
Copy link
Contributor

With geomtries being hashable in 2.0 the weakrefs could potentially be removed from Cartopy because it looks like we are using them in place of the old non-hashable geometries according to this comment:

But that is not something for a 0.21.1 release, I assume?

@jorisvandenbossche
Copy link
Contributor

2. The most time spent in Shapely seems to be in getting coordinates,

It might be easier to comment inline on a draft PR, but one idea to make this more efficient is to do src_coords = np.asarray(geometry.coords) (inside project_linear) and then further access coordinates in that numpy array, instead of accessing the coordinates from the coords object (since this has a __getitem__ in python that has some overhead).

@greglucas
Copy link
Contributor

Draft PR for discussion purposes: #2083

Good idea @snowman2, unfortunately it didn't help much. It looks like quite a bit of time is spent in isinstance and hasattr checks within there. I think we need to try and gather all the points we can right away and do a transform with the entire geometry rather than transforming each point separately. I think it is doable, but will likely need a bit of thought.

@mraspaud
Copy link

mraspaud commented Feb 6, 2023

So shapely 2.0.0 is out since dec 12 2022, and as far as i understand the fixes for this are still pending review. So I thought I could make a small PR to pin shapely to <2.0.0 in the meanwhile. Would that be ok?

@greglucas
Copy link
Contributor

@mraspaud, Cartopy should be compatible with shapely 2.0.0 now are you still seeing issues? Please open a new issue with details if so.

@EJFielding
Copy link

I am using Cartopy installed by the MacPorts system and it has installed Cartopy 0.21.0_0 with shapely 2.0.1_1. This combination is having the issue mentioned above

   import cartopy
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/site-packages/cartopy/__init__.py", line 107, in <module>
    import cartopy.crs  # noqa: E402  module-level imports
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/site-packages/cartopy/crs.py", line 27, in <module>
    import cartopy.trace
  File "lib/cartopy/trace.pyx", line 56, in init cartopy.trace
ImportError: cannot import name lgeos

Is this combination supposed to be fixed?

@QuLogic
Copy link
Member

QuLogic commented May 16, 2023

That line was fixed in 0.21.1, #2110.

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

No branches or pull requests

8 participants