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

Conversions between GeoTools GridCoverage2D and GeoTrellis Raster types #1502

Merged
merged 28 commits into from Jun 6, 2016

Conversation

Projects
None yet
2 participants
@lossyrob
Member

lossyrob commented Jun 1, 2016

Supersedes #1444

jamesmcclain and others added some commits Apr 8, 2016

Fix AWT Translation Bug
According to the documentation[1], DataBuffer.TYPE_BYTE corresponds to
"unsigned byte" not "byte".

1. https://docs.oracle.com/javase/7/docs/api/java/awt/image/DataBuffer.html
PR #1444 Review: Improve NODATA Handling
Since all bands of Geotrellis MultibandTiles must all have the same
cellType and NODATA value, that particular type is not a good candidate
for representing the data in a GeoTools GridCoverage2D.  The best analog
of a GeoTools GridCoverage2D is an Array of Geotrellis Rasters.

Also, the converter now emits *ConstantNoDataCellType when it can.
PR #1444 Review: Raster to GridCoverage2D Fixes
The Raster[Tile] to GridCoverage2D transformation provides best-guess
ColorModels to the created GridCoverage2D objects.  The
Raster[MultibandTile] to GridCoverage2D transformation does not attempt
to provide ColorModel information.  NODATA metadata is now correct in
both the Tile case and MultibandTile case.
PR 1444 Review: Improve NODATA Handling
Since all bands of Geotrellis MultibandTiles must all have the same
cellType and NODATA value, that particular type is not a good candidate
for representing the data in a GeoTools GridCoverage2D.  The best analog
of a GeoTools GridCoverage2D is an Array of Geotrellis Rasters.

Also, the converter now emits *ConstantNoDataCellType when it can.
PR 1444 Review: Raster to GridCoverage2D Fixes
The Raster[Tile] to GridCoverage2D transformation provides best-guess
ColorModels to the created GridCoverage2D objects.  The
Raster[MultibandTile] to GridCoverage2D transformation does not attempt
to provide ColorModel information.  NODATA metadata is now correct in
both the Tile case and MultibandTile case.

@lossyrob lossyrob referenced this pull request Jun 1, 2016

Closed

Add Support for the GridCoverage2D Type #1444

11 of 11 tasks complete
@jamesmcclain

This comment has been minimized.

Show comment
Hide comment
@jamesmcclain

jamesmcclain Jun 3, 2016

Member

+1 This looks good to me. There is quite a bit of test coverage and I was able to confirm that this produces GridCoverage2Ds which are compatible with GeoWave and viewable in the GeoServer.

I think that a valuable contribution would have been a concrete suggestion for compactifying some of the larger chunks of code, but unfortunately -- to the best of my knowledge -- the visual/textual similarity between different blocks of code is not necessarily easily addressed in a statically-typed language without using higher-order functions (or lisp-like macros).

Member

jamesmcclain commented Jun 3, 2016

+1 This looks good to me. There is quite a bit of test coverage and I was able to confirm that this produces GridCoverage2Ds which are compatible with GeoWave and viewable in the GeoServer.

I think that a valuable contribution would have been a concrete suggestion for compactifying some of the larger chunks of code, but unfortunately -- to the best of my knowledge -- the visual/textual similarity between different blocks of code is not necessarily easily addressed in a statically-typed language without using higher-order functions (or lisp-like macros).

ProjectedRaster(toRaster(bandIndex), crs)
case None =>
// Default LatLng
ProjectedRaster(toRaster(bandIndex), LatLng)

This comment has been minimized.

@jamesmcclain

jamesmcclain Jun 3, 2016

Member

I wonder if it is worth emitting a warning here and/or giving a big warning in the source code that a CRS is being assumed.

@jamesmcclain

jamesmcclain Jun 3, 2016

Member

I wonder if it is worth emitting a warning here and/or giving a big warning in the source code that a CRS is being assumed.

*/
object GridCoverage2DConverters {
/**
* Given a [[GridCoverage2D]] and an index, this function

This comment has been minimized.

@jamesmcclain

jamesmcclain Jun 3, 2016

Member

These ScalaDocs comments will produce warnings (sorry).

@jamesmcclain

jamesmcclain Jun 3, 2016

Member

These ScalaDocs comments will produce warnings (sorry).

@jamesmcclain

This comment has been minimized.

Show comment
Hide comment
@jamesmcclain

jamesmcclain Jun 3, 2016

Member

I think that geotools needs to be added to this list in order for the documentation to be buildable with the sbt doc command.

Member

jamesmcclain commented Jun 3, 2016

I think that geotools needs to be added to this list in order for the documentation to be buildable with the sbt doc command.

@jamesmcclain jamesmcclain referenced this pull request Jun 6, 2016

Merged

Add Support for GeoTools SimpleFeature #1495

4 of 4 tasks complete

@lossyrob lossyrob merged commit aa46532 into locationtech:master Jun 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lossyrob

This comment has been minimized.

Show comment
Hide comment
@lossyrob

lossyrob Jun 6, 2016

Member

@jamesmcclain good call. Can you do that in you do that in your SimpleFeature PR?

Member

lossyrob commented Jun 6, 2016

@jamesmcclain good call. Can you do that in you do that in your SimpleFeature PR?

@jamesmcclain

This comment has been minimized.

Show comment
Hide comment
@jamesmcclain

jamesmcclain Jun 7, 2016

Member

@jamesmcclain good call. Can you do that in you do that in your SimpleFeature PR?

Indeed.

Member

jamesmcclain commented Jun 7, 2016

@jamesmcclain good call. Can you do that in you do that in your SimpleFeature PR?

Indeed.

@lossyrob lossyrob added this to the 1.0 milestone Oct 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment