Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
CRS spec definition for version 0.1 #25
CRS spec definition for version 0.1 #25
Changes from 3 commits
2ddcb90
4cdc908
fc13fd9
15e3fa0
9aebaa3
a15b1ac
6caeda0
dfddc3e
6462514
a4db6c3
152fb6b
4e80aab
cc7ae18
b4f956c
8ce7812
beafe33
ea0bf1b
6fd384d
869f335
981fca7
44dca93
aedb19b
de792bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Making this sound a bit more spec-like.
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.
Added #26 for general discussion on officially adopting this kind of language.
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.
Do we need/want to support both 2015 and 2019? What are the advantages of doing so? In general I prefer just one way to do things, but am not an expert on these.
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.
Given that this is a new effort, I think there is something to be said for directly going with only WKT2:2019.
I suppose the main disadvantage is that this new format is not yet as widely supported in libraries, making it more difficult to write compliant geoparquet files.
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.
We did a restriction with WKT2 by removing WKT1. The difference between WKT2 revisions is not really big, and it is well managed by proj4. Thus, I decided to go with all the WKT2 “flavors”, so it’s easier to write files in geoparquet.
I also want to avoid with this decision that people start creating geoparquet files non compliant with non supported WKT2 strings. It might happen that someone use a WKT2 2015 string without realizing of the mistake. If it happens with a large service and a huge amount of datasets out there, the only way to fix it is with another revision of geoparquet spec to add it
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 lean towards going just WKT2:2019 to start, and then see if there are implementations that aren't able to handle it, and then we can loosen it.
The main thing I want to be sure of is that EPSG:4326 has just one representation. So If 2015 vs 2019 for EPSG:4326 are different then I think it'd be really good to just do 2019. My thinking is that we shouldn't make implementations have to check for two different strings that mean the same thing (like if you have proj there).
Ideally we'd have some sort of validator that checks on if it's a 2019 string so we'd catch if a large service starts to use it wrong, so we wouldn't need a spec revision.
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.
Note that because of the dynamic nature of "EPSG:4326" (ensemble datum that gets updated over time), the exact representation of it actually depends on the version of the EPSG database you are using. See eg the comment at #24 (comment), where our example parquet file already changed because of this
Just checking, and it seems that at least PROJ has functionality to check the dialect of a WKT string (although I don't directly see this exposed in pyproj)
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.
The issue is that we cannot guarantee every tool writing geoparquet file has a proper validator in place.
Anyways, it looks like I'm the only one supporting this idea 😄 and it's 0.1, so let's move on with WKT:2019 and we can revisit it in the future.
Really appreciate your feedback here.
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.
Well, we don't need every tool writing geoparquet to have a proper validator in place. We just need one that the implementor creating a tool can use to check their output to ensure it aligns, and/or that data users can use to then give feedback to the implementor.
And more than happy to revisit - as with other discussions I think it's easier to come back and make things looser than it is to try to tighten later.
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 feel like we should mention here that our axis order overrides this?
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.
We already have a specific section for coordinate order where we specify that. Did you see it? Or do you just want to emphasize here too?
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.
Yeah, I saw it - I think we can refer to that section. I just think we need to call it out here. Acknowledge it's a bit confusing. We say here 'use the crs for latitude/longitude', and then below we say 'but do it as longitude/latitude'.
I suppose alternatively we don't actually mention lat/long or or long/lat here - we just that we recommend 4326 for widest interoperability.
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.
+1
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 see your point now 👍