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

possible fix for always assuming WGS84 #157

Closed

Conversation

AndrewAnnex
Copy link
Contributor

This PR is slightly related to developmentseed/cogeo-mosaic#234 and developmentseed/cogeo-mosaic#233

Currently morecantile assumes WGS84 as the geographic_crs for most cases unless the user explicitly over-rides it via the api. However, saving TMSs doesn't always preserve the geographic_crs in json representations (it's not actually an allowed field from the TMS v2 spec, or rather not one explicitly encouraged) so depending on the situation there are cases when the the main CRS is defined for a non-earth celestial body, but the geographic_crs parameter will return 4326 depending on how the TMS is defined.

I think however the geographic crs is always discoverable directly from the CRS of the tms. If given a projected CRS, .geodetic_crs will always return a value, if given a GEOGCRS then the geographic_crs is essentially the same anyways. This PR (should) preserve the ability to allow explicit overrides, but will default to one appropriate to the CRS if not defined. I am not sure how robust this idea is though, so please throw stones at this/think of things I should test for

AndrewAnnex and others added 3 commits October 4, 2024 08:38
add check on geographic_crs, if None raise a ValueError

Co-authored-by: Vincent Sarago <[email protected]>
@AndrewAnnex
Copy link
Contributor Author

@vincentsarago what's your current thinking for this pr given the other work in rio-tiler and etc?

@vincentsarago
Copy link
Member

@AndrewAnnex I think we're good to merge, just need to update the changelog!

@vincentsarago
Copy link
Member

so in fact I cannot Merge this PR 😬

I didn't realized but with #152 and this PR what you're trying to do is to ease the fact that you're storing _geographic_crs within the TMS document. Sadly this should not be authorized (because it breaks the spec)

@AndrewAnnex
Copy link
Contributor Author

closing now that #158 is merged

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