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

ColorMap => String #1512

Merged
merged 6 commits into from Jun 14, 2016

Conversation

Projects
None yet
2 participants
@fosskers
Contributor

fosskers commented Jun 7, 2016

TODO

  • Basic functionality
  • Move breaksString into trait and inherit
  • Special definition for IntCachedColorMap
  • Tests

Motivation

When using the render output module in the ETL process, one must provide a breaks argument, or all tiles will be rendered grayscale. Example:

--output render -O encoding=png path=file:///home/colin/tiles/{name}/{z}-{x}-{y}.png breaks="23:cc00ccff;30:aa00aaff;120:ff0000ff"

The problem is that for datasets which don't have well-defined colour breaks ahead of time like nlcd, there's no way to know what your break string should be for input into ETL. A ColorMap object can be created easily given RDD.colorBreaks and then rendered to a PNG within Scala code, but there is no way to know what String would represent that ColorMap from the outside. This chicken-and-egg problem requires a greater fix to how ETL accepts breaks information from the outside, but this is a start.

(ColorMap => String) First iteration
- Fairly hacky, but demonstrates it's possible.
@fosskers

This comment has been minimized.

Show comment
Hide comment
@fosskers

fosskers Jun 7, 2016

Contributor

To test:

sbt
project raster
console
import geotrellis.raster.render.ColorMap
val m = ColorMap.fromString("23:cc00ccff;30:aa00aaff;120:ff0000ff").get
ColorMap.breaksString(m.breaksMap)
Contributor

fosskers commented Jun 7, 2016

To test:

sbt
project raster
console
import geotrellis.raster.render.ColorMap
val m = ColorMap.fromString("23:cc00ccff;30:aa00aaff;120:ff0000ff").get
ColorMap.breaksString(m.breaksMap)
@fosskers

This comment has been minimized.

Show comment
Hide comment
@fosskers

fosskers Jun 7, 2016

Contributor

Really, the best thing would be for ColorMap to be polymorphic on its internal number type:

trait ColorMap[T] extends Serializable { ... }

And then duplicate code between IntColorMap and DoubleColorMap could be removed. Then, the ColorMap trait could ask for a def breaksMap: Map[T,Int], and breaksString could be moved into the trait defined as:

def breaksString: String = {
  breaksMap  // was `breaksToColors` passed in as an argument
       .toStream
       .map({ case (k,v) => s"${k}:${Integer.toHexString(v)}"})                                           
       .mkString(";")
}

The huge downside of this would be that client code would have to care about the parameterized ColorMap[T], unless we used some cool type aliasing so nothing breaks.
Thoughts, @lossyrob @echeipesh ?

@lossyrob why I'm not in favour of just defining the .toString this way is because it's considered bad form in Haskell-land to have custom output for the show :: Show t => t -> String function, which is similar to toString. Typically show gives you the raw serialized form of the object, such that:

read . show == id

and other "pretty printing" functions are given obvious names.

Contributor

fosskers commented Jun 7, 2016

Really, the best thing would be for ColorMap to be polymorphic on its internal number type:

trait ColorMap[T] extends Serializable { ... }

And then duplicate code between IntColorMap and DoubleColorMap could be removed. Then, the ColorMap trait could ask for a def breaksMap: Map[T,Int], and breaksString could be moved into the trait defined as:

def breaksString: String = {
  breaksMap  // was `breaksToColors` passed in as an argument
       .toStream
       .map({ case (k,v) => s"${k}:${Integer.toHexString(v)}"})                                           
       .mkString(";")
}

The huge downside of this would be that client code would have to care about the parameterized ColorMap[T], unless we used some cool type aliasing so nothing breaks.
Thoughts, @lossyrob @echeipesh ?

@lossyrob why I'm not in favour of just defining the .toString this way is because it's considered bad form in Haskell-land to have custom output for the show :: Show t => t -> String function, which is similar to toString. Typically show gives you the raw serialized form of the object, such that:

read . show == id

and other "pretty printing" functions are given obvious names.

@fosskers

This comment has been minimized.

Show comment
Hide comment
@fosskers

fosskers Jun 7, 2016

Contributor

Another trip-up: the user-level API doesn't expose IntColorMap, etc, only ColorMap. The colouring function has to be visible to the trait. You could put it in the companion object (as I've done initially here) but then the arguments to it must also be visible to the trait. IntCachedColorMap throws a wrench into that, as it has no Map internally, but still extends ColorMap.

Contributor

fosskers commented Jun 7, 2016

Another trip-up: the user-level API doesn't expose IntColorMap, etc, only ColorMap. The colouring function has to be visible to the trait. You could put it in the companion object (as I've done initially here) but then the arguments to it must also be visible to the trait. IntCachedColorMap throws a wrench into that, as it has no Map internally, but still extends ColorMap.

@lossyrob

This comment has been minimized.

Show comment
Hide comment
@lossyrob

lossyrob Jun 7, 2016

Member

Your hitting up against the need for us not to abstract over Int and Double typed things due to performance implications. We jump through hoops to have Double and Int versions of things because if we try to make it generic, things slow down in very painful ways - boxing is the enemy, and ColorMaps are performance critical code that must not box.

I'm not sure "it not being good form in Haskell" is a good reason to avoid using the method that's there, defined as returning a string representation of the object. One could argue that the string representation of a color map that you are concocting is actually a good representation for a user trying to read what the color map is; however, even if we wanted to differentiate between a "show" toString and some other form (def breaksString: String), we could still use implementations in the concrete classes to take care of the Double vs Int bits and avoid the generic breaksString method you have.

Member

lossyrob commented Jun 7, 2016

Your hitting up against the need for us not to abstract over Int and Double typed things due to performance implications. We jump through hoops to have Double and Int versions of things because if we try to make it generic, things slow down in very painful ways - boxing is the enemy, and ColorMaps are performance critical code that must not box.

I'm not sure "it not being good form in Haskell" is a good reason to avoid using the method that's there, defined as returning a string representation of the object. One could argue that the string representation of a color map that you are concocting is actually a good representation for a user trying to read what the color map is; however, even if we wanted to differentiate between a "show" toString and some other form (def breaksString: String), we could still use implementations in the concrete classes to take care of the Double vs Int bits and avoid the generic breaksString method you have.

@fosskers

This comment has been minimized.

Show comment
Hide comment
@fosskers

fosskers Jun 7, 2016

Contributor

haskell

That was more of an explanation as to my natural want to avoid that pattern, not necessarily that we should do it that way "cause Haskell". The question becomes, where do we want the isomorphism? Between ColorMap.fromString <-> breaksString or ColorMap.fromString <-> toString?

re: my last comment. override toString could work for the inheritance problems. I'm not a fan of the duplicate code, but if this is performance critical than I can see the need.

Contributor

fosskers commented Jun 7, 2016

haskell

That was more of an explanation as to my natural want to avoid that pattern, not necessarily that we should do it that way "cause Haskell". The question becomes, where do we want the isomorphism? Between ColorMap.fromString <-> breaksString or ColorMap.fromString <-> toString?

re: my last comment. override toString could work for the inheritance problems. I'm not a fan of the duplicate code, but if this is performance critical than I can see the need.

@lossyrob

This comment has been minimized.

Show comment
Hide comment
@lossyrob

lossyrob Jun 7, 2016

Member

Gotcha. Yeah I think the argument could be made though that we would want to avoid using toString because it's too generic of a thing, and instead use the breakString with inheritance.

Member

lossyrob commented Jun 7, 2016

Gotcha. Yeah I think the argument could be made though that we would want to avoid using toString because it's too generic of a thing, and instead use the breakString with inheritance.

@fosskers

This comment has been minimized.

Show comment
Hide comment
@fosskers

fosskers Jun 7, 2016

Contributor

Meaning the actual definition of breaksString will be duplicated between the *ColorMap classes for performance reasons? I'll do it that way if that's the prevailing pattern.

Contributor

fosskers commented Jun 7, 2016

Meaning the actual definition of breaksString will be duplicated between the *ColorMap classes for performance reasons? I'll do it that way if that's the prevailing pattern.

@lossyrob

This comment has been minimized.

Show comment
Hide comment
@lossyrob

lossyrob Jun 7, 2016

Member

More to do with why we can't do

trait ColorMap[T] extends Serializable { ... }

and be generic on T.

But code duplication in this case might be tough to avoid, since the code to deal with doubles and ints will look the same (but not with the cached version of int, which you mentioned above).

Member

lossyrob commented Jun 7, 2016

More to do with why we can't do

trait ColorMap[T] extends Serializable { ... }

and be generic on T.

But code duplication in this case might be tough to avoid, since the code to deal with doubles and ints will look the same (but not with the cached version of int, which you mentioned above).

fosskers added some commits Jun 7, 2016

@fosskers fosskers changed the title from [WIP] ColorMap => String to ColorMap => String Jun 7, 2016

@fosskers

This comment has been minimized.

Show comment
Hide comment
@fosskers

fosskers Jun 14, 2016

Contributor

Good to go, @lossyrob .

Contributor

fosskers commented Jun 14, 2016

Good to go, @lossyrob .

@lossyrob lossyrob merged commit 42ab052 into locationtech:master Jun 14, 2016

1 check passed

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

@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