-
Notifications
You must be signed in to change notification settings - Fork 360
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
Ability to read/write color tables for GeoTIFFs encoded with palette photometric interpretation #1802
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 val
s inside the body.
@@ -77,6 +79,30 @@ object TiffTagFieldValue { | |||
fieldValues += TiffTagFieldValue(PlanarConfigurationTag, ShortsFieldType, 1, PlanarConfigurations.PixelInterleave) | |||
fieldValues += TiffTagFieldValue(SampleFormatTag, ShortsFieldType, 1, imageData.bandType.sampleFormat) | |||
|
|||
if(geoTiff.options.colorSpace == ColorSpace.Palette) { | |||
|
|||
val bitsPerSample = imageData.bandType.bitsPerSample |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added compatibility check and associated exception here.
import java.nio.ByteOrder | ||
|
||
import geotrellis.raster.render.IndexedColorMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as note above, for import placement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not finding the comment on import placement. Can you repeat it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.mutable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Again, out of habit hit my "organize formats" keybinding, which I have set up differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
"Colormap without Photometric Interpetation = 3." | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to leave off the curly brackets if it's a single statement method (the if counts as a single statement)
} | ||
|
||
(tiffTags &|-> | ||
TiffTags._basicTags ^|-> | ||
BasicTags._colorMap set arr.toSeq) | ||
} else throw new MalformedGeoTiffException( | ||
} | ||
else throw new MalformedGeoTiffException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry... Accidentally hit auto-format out of habit.
|
||
val arr = Array.ofDim[(Short, Short, Short)](divider) | ||
cfor(0)(_ < divider, _ + 1) { i => | ||
arr(i) = ( | ||
shorts(i).toShort, | ||
shorts(i + divider).toShort, | ||
shorts(i + 2 * divider).toShort | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove space for proper formatting
…pe configurations. Code review cleanup/fixes.
🎉 |
No description provided.