-
Notifications
You must be signed in to change notification settings - Fork 45
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
Focal Operations and Buffer Tile support #563
Conversation
ed2c406
to
29dc373
Compare
core/src/main/scala/org/locationtech/rasterframes/expressions/focalops/SurfaceOp.scala
Outdated
Show resolved
Hide resolved
11dd3d9
to
9f0e79d
Compare
* - mutable version makes sense | ||
* - toBytes needs to encode padding size? | ||
*/ | ||
case class BufferTile( |
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.
TODO: merge into GeoTrellis
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.
2134586
to
602d818
Compare
db29294
to
9cb9c7f
Compare
7684fd8
to
b127329
Compare
6637698
to
c406536
Compare
…unction signature change
5c48898
to
d79a6ed
Compare
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.
🤘 🧙 💪
Can you update the release-notes.md
file under a 0.10
heading? I forgot to ask for that in the last PR too.
*/ | ||
def aspect(cs: CellSize, bounds: Option[GridBounds[Int]] = None, target: TargetCell = TargetCell.All): BufferTile = | ||
self.mapTile(_.aspect(cs, bounds, target)) | ||
} |
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.
Should this go into GT?
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.
@@ -49,6 +51,7 @@ class TileUDT extends UserDefinedType[Tile] { | |||
StructField("cols", IntegerType, false), | |||
StructField("rows", IntegerType, false), | |||
StructField("cells", BinaryType, true), | |||
StructField("gridBounds", gridBoundsEncoder[Int].schema, true), |
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.
Yes! This is what I meant by making "everything a buffered tile".... it's effectively enabled now (with perhaps some further propogation)
val ct = CellType.fromName(row.getString(0)) | ||
val cols = row.getInt(1) | ||
val rows = row.getInt(2) | ||
val bytes = row.getBinary(3) | ||
val gridBounds = row.getStruct(4, 5).as[GridBounds[Int]] | ||
BufferTile(ArrayTile.fromBytes(bytes, ct, cols, rows), gridBounds) |
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.
One thing I miss about the CatalystSerializers... all this magic index number stuff was in one place.
val (childTile, childCtx) = tileExtractor(child.dataType)(row(input)) | ||
val result = op(childTile) | ||
toInternalRow(result, childCtx) | ||
// TODO: Ensure InternalRowTile is preserved |
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.
This reminds me... if we want to start improving performance (mainly reducing copies), we need to figure out how ensure InternalRowTile <op> R => InternalRowTile
. Either requires changes to how GeoTrellis instantiates destination tiles, or reimplementation of some fundamental operations.
rf_hillshade(tileCol, lit(azimuth), lit(altitude), lit(zFactor)) | ||
|
||
def rf_hillshade(tileCol: Column, azimuth: Column, altitude: Column, zFactor: Column): Column = | ||
Hillshade(tileCol, azimuth, altitude, zFactor) |
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.
💯
""" | ||
uri - STAC API uri | ||
filters - a STAC API Search filters dict (bbox, datetime, intersects, collections, items, limit, query, next) | ||
search_limit - search results convenient limit method |
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.
This might be better as named parameters, or as **kwargs
.
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.
Discoverability when working in a notebook or something would benefit from these parameters being fully spelled out with defaults.
altitude = lit(altitude) | ||
if isinstance(z_factor, (int, float)): | ||
z_factor = lit(z_factor) | ||
return _apply_column_function('rf_hillshade', tile_col, azimuth, altitude, z_factor) |
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.
Nicely done!!
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 there's documentation duplicated. In another universe I would have figured out how to have the Python API generated from the annotations on the Expressions.
@@ -390,6 +390,11 @@ def __init__(self, cells, cell_type=None): | |||
# if the value in the array is `nd_value`, it is masked as nodata | |||
self.cells = np.ma.masked_equal(self.cells, nd_value) | |||
|
|||
# is it a buffer tile? crop it on extraction to preserve the tile behavior |
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 this is the right thing to do, but don't want to hold up the PR for this. Back to the "everything's a buffered tile" perspective, I'd want to explore supporting buffered tile semantics on the Python mirror type.
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.
@metasim yes, for now it is done to preserve the old behavior and not to break anything.
except ValueError as e: | ||
raise ValueError({ | ||
"cell_type": cell_type, | ||
"cols": cols, | ||
"rows": rows, | ||
"cell_data.length": len(cell_data_bytes), | ||
"cell_data.type": type(cell_data_bytes), | ||
"cell_data.values": repr(cell_data_bytes) | ||
"cell_data.values": repr(cell_data_bytes), | ||
"grid_bounds": datum.gridBounds |
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.
🙏
@@ -1,21 +1,21 @@ | |||
# jupyter/scipy-notebook isn't semantically versioned. | |||
# We pick this arbitrary one from Sept 2019 because it's what latest was on Oct 17 2019. | |||
FROM jupyter/scipy-notebook:7a0c7325e470 | |||
FROM jupyter/scipy-notebook:python-3.8.8 |
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.
👍
f803707
to
c7d8f48
Compare
It is a port of #551
TODO:
, blocked by Python binding updates for PySpark 3.1 #562quay.io/daunnc/rasterframes-notebook:0.9.2-SNAPSHOT
Focal Operations notebook screenshot
STAC API Example notebook screenshot
Closes #441
Closes #551
Closes #487
Blocked by locationtech/geotrellis#3419