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

Allow rasterizer to store z value at double precision #2388

Merged
merged 2 commits into from Sep 20, 2017

Conversation

Projects
None yet
3 participants
@moradology
Contributor

moradology commented Sep 19, 2017

This PR allows rasterization to store values at double precision (which is immediately useful for elevation rasters) and allows the z values to be stored with a provided CellType

@moradology moradology force-pushed the moradology:feature/double-rasterizer branch from 907c288 to 8af0d1e Sep 19, 2017

@jamesmcclain

This comment has been minimized.

Member

jamesmcclain commented Sep 20, 2017

I think that this was originally the case, but these values were narrowed for economic reasons.

@echeipesh

This comment has been minimized.

Contributor

echeipesh commented Sep 20, 2017

Yep, this PR parameterizes it so economy and precision can be served.

case class CellValue(value: Double, zindex: Short)
/** Cell value with its zindex and celltype to be used by the rasterizer. */
case class CellValue(value: Double, zindex: Double, celltype: CellType)

This comment has been minimized.

@echeipesh

echeipesh Sep 20, 2017

Contributor

I'm realizing now that the issue was misstated, exposing cellType for zindex per feature actually leads ambiguous behavior: the order in which tiles are merged is undefined and if two priority tiles do not share the same zindex type it is unclear which will be updated and propagated.

So this parameter should be at function invocation level as zindexCellType. Probably ByteConstantNoDataCellType is a nice conservative default value for it.

@moradology moradology force-pushed the moradology:feature/double-rasterizer branch from 26b8e77 to 6c7227f Sep 20, 2017

@moradology

This comment has been minimized.

Contributor

moradology commented Sep 20, 2017

@echeipesh I believe I've addressed the issue you raised - let me know if I'm missing something

@echeipesh echeipesh merged commit 331115b into locationtech:master Sep 20, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment