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 geographic crs attribute #158

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

vincentsarago
Copy link
Member

@vincentsarago vincentsarago commented Oct 10, 2024

ref #157 #152

what do you think @AndrewAnnex

#152 introduced the fact the _geographic_crs could be stored in the TMS document but that is breaking the TMS specification.

if the proposed solution (relying on crs.geodetic_crs) doesn't work, then I'm not sure what we should do

@AndrewAnnex
Copy link
Contributor

@vincentsarago yeah I was having to store _geographic_crs because it was getting set to WGS84 by default, and #152 was in keeping with that to avoid breaking changes. I definitely want to get to a place where I am not doing that and the geographic crs get's correctly interpreted from the CRS of the TMS
I think this should work but there might be some weird cases where the axis order/coordinate system changes (like Mars planetographic with northing westing coordinates) and by explicitly setting a geographic_crs within the tms as a separate parameter you can work to avoid that. In any case, I think this is better than assuming 4326/wgs84. But in any case if it's breaking the specification that's something to work on fixing. I don't think the Spec says the geographic crs must be 4326 right?

@vincentsarago
Copy link
Member Author

I don't think the Spec says the geographic crs must be 4326 right?

Yeah, the fact that we need the geographic CRS is strictly for morecantile internals:

lng, lat = self._to_geographic.transform(x, y)

@cached_property
def bbox(self):
"""Return TMS bounding box in geographic coordinate reference system."""
left, bottom, right, top = self.xy_bbox
return BoundingBox(
*self._to_geographic.transform_bounds(
left,
bottom,
right,
top,
densify_pts=21,
)
)

if not projected:
west, south, east, north = self._to_geographic.transform_bounds(
west, south, east, north, densify_pts=21
)

with open(tileset, "r") as f:
ts = TileMatrixSet.model_validate_json(f.read())

if not pyproj.CRS.from_epsg(4326) == ts.geographic_crs:
Copy link
Member Author

Choose a reason for hiding this comment

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

for some TMS, the geographic_crs returned by tms._pyproj_crs.geodetic_crs isn't strictly equal to WGS84, but by checking the geographic transformation of the tms bbox we can see they give the same result

@vincentsarago
Copy link
Member Author

@AndrewAnnex are we good here?

@vincentsarago vincentsarago force-pushed the feature/refactor-geographic-crs branch from 923e9d3 to 4aa83ec Compare October 15, 2024 16:43
@AndrewAnnex
Copy link
Contributor

@vincentsarago yeah I think this is looking good now, I don't think you need to do quite as much work in the tests with transforming the bboxes though as to and from geographic functions both enforce always_xy order, but think this is about right.

let's lift the test from my other pr into this pr and see if anything needs to be changed, or I can make a follow on PR:
https://github.com/developmentseed/morecantile/pull/157/files#diff-0adb667cd397bd56edce10eec11e1b10821740a10d72bd148b09645fc9108968R293

@vincentsarago
Copy link
Member Author

let's merge this one and then you can create a new PR with the tests

@vincentsarago vincentsarago merged commit 3fa4e2e into main Oct 15, 2024
6 checks passed
@vincentsarago vincentsarago deleted the feature/refactor-geographic-crs branch October 15, 2024 18:26
@AndrewAnnex
Copy link
Contributor

@vincentsarago something isn't quite right with TMS with inverted axes, I am investigating but will need to stop and do other things so unless I see a quick fix I'll push a pr with failing tests

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