Added resampling method for 3 layers and updated tests #101
Conversation
city_metrix/layers/layer.py
Outdated
| @@ -325,6 +325,18 @@ def get_stats_funcs(stats_func): | |||
| return [stats_func] | |||
|
|
|||
|
|
|||
| def set_resampling_method(image: ee.Image, resampling_method: str): | |||
| valid_raster_resampling_methods = ['bilinear', 'bicubic'] | |||
There was a problem hiding this comment.
Should you restrict the resampling methods on the dataset level instead of in this function? I think some datasets wouldn't want these resampling method (e.g. if they're not continuous).
There was a problem hiding this comment.
Wherever possible, I am striving to implement as much code as possible in layer functions so that the logic does not need to be separately implemented in multiple sub-classes.
As you mentioned, this function can only be used for continuous raster. I am renaming the function and adding documentation to further clarify the point.
city_metrix/layers/layer.py
Outdated
| raise ValueError(f"Invalid resampling method ('{resampling_method}'). " | ||
| f"Valid methods: {valid_raster_resampling_methods}") | ||
|
|
||
| data = image.resample(resampling_method) |
There was a problem hiding this comment.
I'm not sure how this would play into, but in get_image_collection, you might accidentally trigger some sort of resampling by virtue of reading a raster at a given scale (which may be different than the native resolution of the raster). Do you need to make sure you're resampling correctly in the Xee read, or does it seem to handle it properly by calling this function here?
There was a problem hiding this comment.
Resampling needs to be specified on the images before calling XEE as far as I've found with my investigations.
city_metrix/layers/albedo.py
Outdated
| albedo_mean = s2_albedo.reduce(ee.Reducer.mean()) | ||
| if self.resampling_method is not None: | ||
| albedo_mean = (s2_albedo | ||
| .map(lambda x: set_resampling_method(x, self.resampling_method)) |
There was a problem hiding this comment.
small nitpick to reduce code repetition, but I think because of how GEE works, you could just do this more like
resampled_albedo = s2_albedo
if self.resampling_method is not None:
resampled_albedo = s2_albedo
.map(lambda x: set_resampling_method(x, self.resampling_method))
albedo_mean = (resampled_albedo.
.reduce(ee.Reducer.mean())
)
Maybe that's not any better though... your choice
There was a problem hiding this comment.
Alternatively, based on comment below, maybe you could just define the resampling in the Xee call and just pass it along to get_image_collection?
There was a problem hiding this comment.
As far as I have determined, XEE does not yet directly support resampling. Resampling must instead be specified on the image.
There was a problem hiding this comment.
I found a way to fully implement in the called function.
|
Thank your for the thoughtful review. The new code is functionally improved and now avoids a pixel offset issue. |
Tested on my local box.
Test results for a part of Amsterdam are located in s3://wri-cities-heat/demo/kenn_transfer/CIF_316_test_result_tif_files.zip including Albedo, DEM.DSM before/after tiff files. Also include a QGIS file for easy examination of the differences.
I didn't find a functional way to handle the no resampling option that GEE would accept.