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

refactor attribute and remove TMS from BaseReader init, and remove min/maxzoom from info response #745

Closed
wants to merge 1 commit into from

Conversation

vincentsarago
Copy link
Member

@vincentsarago vincentsarago commented Oct 7, 2024

closes #744

This PR does:

  • refactors the SpatialMixin base class

    • removes tms attribute
    • removes geographic_crs attribute
    • removes minzoom and maxzoom properties
    • add get_zooms(tms: TileMatrixSet) method
    • change geographic_bounds property to method geographic_bounds(crs: CRS)
    • update tile_exists and add tms argument
  • changes .tile() method and add tms argument

  • refactors the info() method

    • return dataset bounds (instead of geographic bounds)
    • add crs metadata (epsg or wkt)
  • removes models.SpatialInfo and models.Bounds

  • removes minzoom/maxzoom attributes in MultiBaseReader and MultiBandReader

  • starts a migration guide

# before
import morecantile
from rio_tiler.io import Reader

tms = morecantile.tms.get("WebMercatorQuad")
with Reader("cog.tif", tms=tms, geographic_crs=tms.rasterio_geographic_crs) as src:
    _ = src.tile_exists(0, 0, 0)
    _ = src.tile(0, 0, 0)
    _ = src.minzoom
    _ = src.maxzoom
    _ = src.geographic_bounds

    info = src.info()
    assert info.bounds == src.geographic_bounds
    assert info.minzoom is not None
    assert info.maxzoom is not None

# now
import morecantile
from rio_tiler.io import Reader

tms = morecantile.tms.get("WebMercatorQuad")
with Reader("cog.tif") as src:
    _ = src.tile_exists(0, 0, 0, tms=tms)
    _ = src.tile(0, 0, 0, tms=tms)
    _, _ = src.get_zooms(tms)
    _ = src.geographic_bounds(tms.rasterio_geographic_crs)
    
    info = src.info()
    assert info.bounds == src.bounds == src.dataset.bounds
    assert info.crs == src.crs == src.dataset.crs
    assert not hasattr(info, "minzoom")
    assert not hasattr(info, "maxzoom")


@cached_property
def geographic_bounds(self) -> BBox:
def geographic_bounds(self, crs: CRS = WGS84_CRS) -> BBox:
Copy link
Contributor

@AndrewAnnex AndrewAnnex Oct 7, 2024

Choose a reason for hiding this comment

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

I think the default CRS here should not be WGS84 but the CRS returned from self.crs.geodetic_crs As a asset probably should have a CRS, the geographic crs here should be appropriate to the CRS of the dataset itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AndrewAnnex the problem is that rasterio CRS class doesn't have geodetic_crs 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh well that would be a problem. But I have an idea and will post in the new issue

@@ -176,10 +161,13 @@ def _get_descr(ix):
else:
nodata_type = "None"

crs_string = (
f"EPSG:{self.crs.to_epsg()}" if self.crs.to_epsg() else self.crs.to_wkt()
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm why not opt instead to use a urn/uri rather than attempt a epsg conversion here? those should work with the other authorities and be relatively compact. Failing through to wkt makes sense but also think wkt2_2019 should be preferred as it's more verbose and complete.

@vincentsarago
Copy link
Member Author

I feel I went too far with this PR (thanks for a good night of 🛏️)

I've taken another approach over #746

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.

replace geographic_bounds with bounds and removes maxzoom, minzoom from info() response
2 participants