Switch NDArrayImage dimension order to (band, y, x)
#32
Conversation
There was a decent amount of duplication between Image subclasses in handling ufuncs and postprocessing results that might be tuples. To simplify things, apply_ufunc_across_bands is now defined by Image, with preprocessing and postprocessing handled by subclasses. This switch means that Numpy arrays are now handled through xr.apply_ufunc, which is supported by xarray even though it's slightly odd.
src/sknnr_spatial/datasets/_base.py
Outdated
| with rasterio.open(path) as src: | ||
| band = src.read(1) | ||
| arr = band if arr is None else np.dstack((arr, band)) | ||
| if arr is None: | ||
| arr = np.empty((len(file_paths), *band.shape), dtype=band.dtype) |
There was a problem hiding this comment.
Is there any possibility we're going to run into typing issues if the first image is, say, uint8 and subsequent bands are float? Previously, I think np.dstack would take care of that automatic promotion to float, but this may not?
There was a problem hiding this comment.
Yeah, you're absolutely right! It looks like this will down-cast subsequent band arrays to make them fit in the arr dtype, so floats would get rounded. That would've been a tough bug to track down!
I rewrote this closer to the original but using concatenate instead of dstack, and added a test to make sure that loading an int and float raster gives you a float raster back.
There was a problem hiding this comment.
Perfect! And of course you went the extra step and wrote an awesome test for this as well! OK to merge ...
|
Thanks for the review @grovduck! Good catch on the dtype issue! |
Closes #29 by making
NDArrayImageuse shape(band, y, x), which is consistent with multi-band images loaded byrasterioorrioxarray. Dataset loading and tests were updated to work with the new dimension order.It's worth noting that internally,
_ImageChunkstill handles images in(y, x, band)order becausexr.apply_ufuncalways moves the core dimension (band) to the last axis. This means thatNDArrayImagealso has to transpose arrays before they go into the ufunc, and both image types need to transpose the ufunc outputs back to(band, y, x).To reduce some duplication introduced by that change, I refactored
apply_ufunc_across_bandsfrom theImagesubclasses into the base class, with the type-specific functionality now handled by preprocessing and postprocessing functions. This means that both image types are now handled identically byxr.apply_ufunc, which further simplifies things. It does feel a little weird callingxr.apply_ufuncwith Numpy arrays, but it's a documented feature and I'm sure the overhead of passing it through is negligible.