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

Orthographic threshold reduction #1951

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

Conversation

ludwigVonKoopa
Copy link
Contributor

Related issue : #1907

This PR changes Orthographic projection threshold to solve tiles loading error.
With actual threshold value, when using tiles with enough precision, the projection boundaries are eroded too much, and result in some tiles data not loaded :

import matplotlib.pyplot as plt
import cartopy.crs as ccrs
from cartopy.io.img_tiles import Stamen, GoogleWTS, GoogleTiles
import matplotlib.patches as mpatches
import shapely.geometry as sgeom

tiler = Stamen(style="terrain-background", cache="/tmp/tiles/")

fig = plt.figure(figsize=(12,7), dpi=120)

proj1 = ccrs.Orthographic(0, 45)
proj2 = ccrs.Orthographic(0, 45)
proj2.threshold /= 20
titles = [f"threshold = {p.threshold:,.2f}" for p in (proj1, proj2)]
for p, proj, title in zip(range(2), [proj1, proj2], titles):

    ax_pla = fig.add_subplot(2, 2, 1+p, projection=ccrs.PlateCarree(), frameon=False)
    ax_ort = fig.add_axes([0.5*p, 0.1, 0.5, 0.5], projection=proj)

    bounds_proj = sgeom.Polygon(proj.boundary)
    geom = ax_ort._get_extent_geom(tiler.crs)
    img, extent, origin = tiler.image_for_domain(geom, 4)

    for ax in [ax_pla, ax_ort]:
        ax.imshow(img, extent=extent, origin=origin, transform=tiler.crs, regrid_shape=500)
        c_geom = ax.add_geometries([geom], crs=tiler.crs, color="red", alpha=0.3, zorder=30)
        c_bounds = ax.add_geometries([bounds_proj], crs=proj, color="green", alpha=0.3, zorder=40)
    ax_pla.set_title(title)
    
geo2 = mpatches.Patch(color='green', label="ortho boundaries")
geo1 = mpatches.Patch(color='red', label='eroded boundaries')
ax_pla.legend(handles=[geo1, geo2], loc=(-0.3,-0.5))

image

I did not add tests with this PR, because i don't see how i could easily tests this.

I could add a test which

  • create a black image
  • paint tiles which pass through correct extent
  • project this tiles on orthographic
  • and check in the final image if there is black.

But this may be too much. What do you think ?

Thanks

@greglucas
Copy link
Contributor

Looks like you'll have to update some images that are causing the test failures. I think all of the new images look like improvements to me, which is good :)

For the multiple projections one, we may want to just increase the tolerance a bit rather than updating that image, since it is only a small difference for one image test. (We should really break this out into separate images later on)

@QuLogic
Copy link
Member

QuLogic commented Dec 8, 2021

We should really break this out into separate images later on

Oh, wait, have I not PR'd this already? I should. #1964

@greglucas
Copy link
Contributor

@ludwigVonKoopa, the images were just updated, so you're going to have to rebase this and likely need to update the Orthographic test image. Can you also remove the 0.9999*a in the projection definition while you're at it?

@greglucas
Copy link
Contributor

@ludwigVonKoopa, I still think this looks like a good update if you're interested in getting it in. I'm guessing you'll need to update the Orthographic image tests already present in the test suite. One additional request would be to put a comment above, saying something about why this should be the threshold so someone else doesn't come and change it later unknowingly.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

4 participants