-
Notifications
You must be signed in to change notification settings - Fork 108
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
Conversation
…n/maxzoom from info response
|
||
@cached_property | ||
def geographic_bounds(self) -> BBox: | ||
def geographic_bounds(self, crs: CRS = WGS84_CRS) -> BBox: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
😬
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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.
I feel I went too far with this PR (thanks for a good night of 🛏️) I've taken another approach over #746 |
closes #744
This PR does:
refactors the
SpatialMixin
base classtms
attributegeographic_crs
attributeminzoom
andmaxzoom
propertiesget_zooms(tms: TileMatrixSet)
methodgeographic_bounds
property to methodgeographic_bounds(crs: CRS)
tile_exists
and addtms
argumentchanges
.tile()
method and addtms
argumentrefactors the
info()
methodcrs
metadata (epsg or wkt)removes
models.SpatialInfo
andmodels.Bounds
removes
minzoom/maxzoom
attributes in MultiBaseReader andMultiBandReader
starts a migration guide