Repository navigation
Ability to read/write color tables for GeoTIFFs encoded with palette photometric interpretation - #1802
Conversation
lossyrob
left a comment
lossyrob
left a comment
There was a problem hiding this comment.
Looking good, just a couple of nitpicks.
| it("should write color map when photometric interpretation is 'Palette'") { | ||
| val hundreds = createConsecutiveTile(10).map(_ - 1).convert(ByteCellType) | ||
|
|
||
| val colorMap = ColorRamps.HeatmapBlueToYellowToRedSpectrum //ClassificationBoldLandUse |
There was a problem hiding this comment.
Remove comment (or make other test case that uses it?)
| private def upsample(c: Int) = (c * 257).toShort | ||
|
|
||
| /** Creates an IndexColorMap from sequence of RGB short values. */ | ||
| private[raster] def fromTiffPalette(tiffPalette: Seq[(Short, Short, Short)]) = new IndexedColorMap( |
There was a problem hiding this comment.
not sure there's a need to make this private to [raster], is there a good reason to hide it?
There was a problem hiding this comment.
Was't sure if in this project making it public meant we had to commit to it as an API. But I'll make public.
| for { | ||
| cmap ← geoTiff.options.colorMap | ||
| palette = IndexedColorMap.toTiffPalette(cmap) | ||
| size = math.min(palette.size, divider) |
There was a problem hiding this comment.
Using = inside a for comprehension isn't idiomatic to the library. Might cause confusion because if it's in the for it seems like it should be a map or flatMap, that's how I read it anyway. Could be extracted to vals inside the body.
|
|
||
| if(geoTiff.options.colorSpace == ColorSpace.Palette) { | ||
|
|
||
| val bitsPerSample = imageData.bandType.bitsPerSample |
There was a problem hiding this comment.
Unclear whether to throw here if it's invalid to have for this BandType, or on creation. I would say perhaps on creation.
There was a problem hiding this comment.
Added compatibility check and associated exception here.
|
|
||
| import java.nio.ByteOrder | ||
|
|
||
| import geotrellis.raster.render.IndexedColorMap |
There was a problem hiding this comment.
Same as note above, for import placement
There was a problem hiding this comment.
I'm not finding the comment on import placement. Can you repeat it?
There was a problem hiding this comment.
Ah, I hit "preview" to look at the code, and then failed to hit "Add review comment"...
The order of imports should be as follows:
// GeoTrellis imports, alphabetical
import geotrellis.raster._
import geotrellis.vector._
import geotrellis.vector.io._
// Third party imports, alphabetical
import org.apache.spark._
import spray.json._
// Java and Scala imports, alphabetical
import java.nio._
import scala.collections.mutableThere was a problem hiding this comment.
Got it. Again, out of habit hit my "organize formats" keybinding, which I have set up differently.
There was a problem hiding this comment.
Are you using IntelliJ? Curious, a lot of people do, but most of the Azavea team doesn't...useful to know if we try it out and have questions :)
| else throw new MalformedGeoTiffException( | ||
| "Colormap without Photometric Interpetation = 3." | ||
| ) | ||
| } |
There was a problem hiding this comment.
Better to leave off the curly brackets if it's a single statement method (the if counts as a single statement)
| BasicTags._colorMap set arr.toSeq) | ||
| } else throw new MalformedGeoTiffException( | ||
| } | ||
| else throw new MalformedGeoTiffException( |
There was a problem hiding this comment.
else belongs on the same line as the close bracket. Would do
} else
throw new MalformedGeoTiffException(
)or do {} for the else body
There was a problem hiding this comment.
Sorry... Accidentally hit auto-format out of habit.
| shorts(i + divider).toShort, | ||
| shorts(i + 2 * divider).toShort | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Remove space for proper formatting
…pe configurations. Code review cleanup/fixes.
|
🎉 |
2 participants
No description provided.