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

fix: Invalid Polygon from Contouring #2463

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Clarmy
Copy link

@Clarmy Clarmy commented Oct 28, 2024

Rationale

Resolve the issue mentioned in #2370
New PR from #2373

Implications

This change will affect the behavior of contourf and essentially fixes a bug in the contourf function.

@rcomer
Copy link
Member

rcomer commented Nov 28, 2024

Hi @Clarmy, thanks for updating here and sorry it has taken me a while to look at this. I was just playing with the branch and found that the new tests do not fail for me against main. So I tried the code from test_contourf_with_conflict_points, and it fails for me with Cartopy 0.23 but passes with Cartopy 0.24. In fact your original code from #2370 also works (albeit slowly) for me with Cartopy 0.24. Can you confirm if you still have an issue with your original data set(s) and Cartopy 0.24?

I do not know what change would have made the difference. We have obviously made changes in this area but none of them specifically targeted that error. I am also looking at different versions of Shapely (2.0.4 to 2.0.6) and Pyproj (3.6.1 to 3.7.0) so possibly something changed outside of Cartopy to fix this.

@Clarmy
Copy link
Author

Clarmy commented Nov 28, 2024

@rcomer Yes, it still fails when I test with cartopy==0.24.1.

import numpy as np
import cartopy
import cartopy.crs as ccrs
import matplotlib.pyplot as plt

print(f"cartopy version: {cartopy.__version__}")


def test_contourf_with_conflict_points():
    """Testing the issue mentioned in #2370"""

    lons = [96.75, 97.0, 97.25]
    lats = np.arange(17.25, 15.9, -0.25)

    lons, lats = np.meshgrid(lons, lats)

    data = [
        [26.9, 43.7, 26.8],
        [33.2, 65.8, 34.6],
        [54.7, 66.9, 55.5],
        [65.3, 65.0, 65.7],
        [65.9, 65.0, 65.6],
        [65.8, 65.2, 65.5],
    ]

    fig = plt.figure()
    ax = fig.add_subplot(projection=ccrs.Mercator())
    ax.contourf(lons, lats, data, levels=[60, 65], transform=ccrs.PlateCarree())


if __name__ == "__main__":
    test_contourf_with_conflict_points()

image

@Clarmy
Copy link
Author

Clarmy commented Nov 28, 2024

@rcomer I created a brand new python=3.10 virtual environment using conda, and only installed the latest version of cartopy==0.24.1 released on pip (without installing any other packages). Then I ran the code directly, and this is the error that occurred. Were your tests based on the code from the main branch rather than the most recent release package on PyPI?

@rcomer
Copy link
Member

rcomer commented Nov 28, 2024

I used a centrally provided environment at my workplace, which was recently updated. That environment uses conda and gets all its packages from conda-forge. It is python 3.12.7.

The test code gives me this:

Figure_1

@Clarmy
Copy link
Author

Clarmy commented Nov 28, 2024

Could you please follow these steps to see if you can reproduce the error I encountered:

$ conda create -n py312 python=3.12 -y
$ conda activate py312
$ pip install -U cartopy
$ python test.py

Here, test.py is the script containing test_contourf_with_conflict_points.

@Clarmy
Copy link
Author

Clarmy commented Nov 28, 2024

@rcomer Or, I think there's an even more convincing way to test this: add test_contourf_with_conflict_points directly to the test cases and see if the latest CI for version 0.24 can pass the tests.

@rcomer
Copy link
Member

rcomer commented Nov 29, 2024

Good idea to use the CI. I made this change so I can test within my fork, and the test fails for every setup. However, that test passes for me locally with the conda environment I created from the conda yml. I am very confused.

@rcomer
Copy link
Member

rcomer commented Nov 29, 2024

I have confirmed that if I install Cartopy with pip in a fresh environment, I get the error. However, if I just make a new conda environment with

conda create -n cartopy-check2 python=3.12 cartopy

I do not get the error.

@rcomer
Copy link
Member

rcomer commented Nov 29, 2024

When installing from conda, I get geos version 3.13 which only came out in September. If I go again with

conda create -n cartopy-check3 python=3.12 cartopy geos=3.12

I reproduce the error. So it seems like the latest geos did something to fix this. The only other difference I see in the two environments is I get a different shapely build. conda-forge tends to rebuild things independently of package maintainers, whereas pypi only has what maintainers build when the package is released.

@Clarmy
Copy link
Author

Clarmy commented Nov 29, 2024

@rcomer Thank you for your testing. Indeed, it seems that the error disappeared when I used geos=3.13. However, there is an issue: C GEOS is only available in conda and cannot be installed via pip. Additionally, the release notes for cartopy 0.22.0 explicitly state that the C GEOS dependency was removed in favor of shapely to simplify the installation of cartopy.

@Clarmy
Copy link
Author

Clarmy commented Nov 29, 2024

Shapely depends on GEOS, and it installs a pre-compiled GEOS wheel during its installation. If the version of GEOS that Shapely is built against could be updated to 3.13, the issue might be resolved.

@Clarmy
Copy link
Author

Clarmy commented Nov 29, 2024

I just took a closer look at the installation section of the Shapely documentation. It mentions that if users want to customize the version of the GEOS package, they can install their preferred version of GEOS and then use pip install shapely --no-binary to install Shapely. So, I'm thinking, could we try re-adding the GEOS dependency in Cartopy, using a pre-compiled method (similar to how Shapely does it) so that Cartopy directly includes GEOS 3.13?

@Clarmy
Copy link
Author

Clarmy commented Nov 29, 2024

While reading through the Shapely code repository, I found their GEOS version definition for the build process: here. Indeed, they are building against version 3.12. So, I submitted a PR to GEOS to upgrade the GEOS version number: shapely/shapely#2187 this PR is merged, then we might only need to adjust the version requirement for Shapely in Cartopy to fix the issue.

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.

2 participants