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

Fix usage of IntCachedColorMap in Indexed PNG encoding #2075

Merged
merged 4 commits into from Mar 20, 2017

Conversation

Projects
None yet
4 participants
@echeipesh
Contributor

echeipesh commented Mar 16, 2017

IntCachedColorMap implementation was not covered by unit tests and turned out to be incorrect.
This PR adds unit tests for its usage and changes the behavior to something that works.

It looks like this class fell through some cracks in the refactors. In reality it leans on implementation of FastMapHistogram to convert pixels directly to colors, which uses a hash table lookup.

In effect we get eC color encoding instead of Log which is provided by IntColorMap

@echeipesh echeipesh added the bug label Mar 16, 2017

@echeipesh echeipesh added this to the 1.1 milestone Mar 16, 2017

@echeipesh

This comment has been minimized.

Contributor

echeipesh commented Mar 16, 2017

@metasim This is the bug you ran into.
Does the test cover the way you were trying to use this feature?

@fosskers

This comment has been minimized.

Contributor

fosskers commented Mar 16, 2017

Right, I recall this not being subject to the BreakMap refactor that the other ColorMap subclasses got.

@metasim

This comment has been minimized.

Member

metasim commented Mar 16, 2017

@echeipesh I believe so. Glad there was actually something behind it.

@echeipesh echeipesh requested a review from pomadchin Mar 17, 2017

val r = createTile(arr)
val h = r.histogram
val colorMap = colorMap1.withNoDataColor(0).withBoundaryType(LessThanOrEqualTo).asInstanceOf[IntColorMap].cache(h)

This comment has been minimized.

@pomadchin

pomadchin Mar 20, 2017

Member

Can we avoid somehow this runtime cast?

@pomadchin

pomadchin approved these changes Mar 20, 2017 edited

Looks fine to me, except these two comments.

h.foreachValue(z => ch.setItem(z, map(z)))
new IntCachedColorMap(colors, ch, options)
val cachedColors = h.values()
cfor(0)( _ < cachedColors.length, _ + 1) { i =>

This comment has been minimized.

@pomadchin

pomadchin Mar 20, 2017

Member

Is h.totalCount would be values count?

This comment has been minimized.

@echeipesh

echeipesh Mar 20, 2017

Contributor

h.totalCount would give me the total number of times any value was seen bytes histogram across all buckets. Not sure if thats useful here. What are you thinking?

This comment has been minimized.

@pomadchin

pomadchin Mar 20, 2017

Member

Eh, nothing smart, that's why i approved it. I tried to avoid somehow calculating length for the second time (as it's already known in fact) and trying to avoid distinct function usage. But no; it's fine as it is.

@echeipesh echeipesh merged commit 1f656ed into locationtech:master Mar 20, 2017

1 check passed

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

@echeipesh echeipesh removed the in progress label Mar 20, 2017

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