Repository navigation
Fix TemporalProjectedExtentCodec to handling proj4 strings when CRS isn't available - #2034
Conversation
|
I'm +1 on this, @echeipesh your review was requested. |
| } | ||
| else { | ||
| rec.put("proj4", crs.toProj4String) | ||
| } |
There was a problem hiding this comment.
Another (breaking) option would be to replace epsg all together with crs, and prefix EPSG codes according to this, falling back to "proj4" strings otherwise.
There was a problem hiding this comment.
With Avro schema migration capabilities (which allow for aliasing), this might not have to be a breaking change. See: https://github.com/locationtech/geotrellis/blob/master/spark/src/main/scala/geotrellis/spark/io/avro/codecs/KeyCodecs.scala#L52
pomadchin
left a comment
pomadchin
left a comment
There was a problem hiding this comment.
Think for consistency https://github.com/metasim/geotrellis/blob/master/spark/src/main/scala/geotrellis/spark/io/avro/codecs/ProjectedExtentCodec.scala should be modified too.
|
@pomadchin Good catch. Will look for a mechanism to share code (independent codec?). @echeipesh are you good with this? |
|
+1, nice that it was kept from being a breaking change. Shame that |
|
@echeipesh Created a shared CRS codec. |
|
D'oh!.... Think this should be handled some other way?
…On Tue, Mar 7, 2017 at 11:22 AM, Grisha ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In spark/src/main/scala/geotrellis/spark/io/avro/codecs/
TemporalProjectedExtentCodec.scala
<#2034 (comment)>
:
> rec.put("extent", extentCodec.encode(temporalProjectedExtent.extent))
- rec.put("epsg", temporalProjectedExtent.crs.epsgCode.get)
+ if(crs.epsgCode.isDefined) {
+ rec.put("epsg", crs.epsgCode.get)
+ }
+ else {
+ rec.put("proj4", crs.toProj4String)
+ }
@metasim <https://github.com/metasim> @lossyrob
<https://github.com/lossyrob> I'm wondering what means breaking here, as
there was no this codec in 1.0.0 O:
|
|
@lossyrob As I'm reading this Avro schemes they fall under schema migration, so that isn't breaking: old records can be read using new versions. The new Probably the moral is that we'll have to release |
|
@metasim for future reference: The PR should be named describing the change, and the body of the PR should say "Fixes #XXXX" - this way they are properly linked in GitHub, and the issue actually closes once the PR is merged. This way (with Fixes comment in a commit, and issue number in PR name) took just a bit to track down, no big deal but just a heads up :) I made changes to the PR title and description |
lossyrob
changed the title
4 participants
Fixes #2033