feat: Implementation of resample_spatial to reproject values

[SerRichard]

Dask implementation of resample spatial using the odc.algo reproject function. I've tried to handle some trickier parts (like infering the affine transformation) with some helper utils. Ready for review when all boxes are ticked!

• Reprojection
• Resampling
• Unit tests

[codecov]

Codecov Report

Merging #103 (57af9be) into main (9f184e5) will increase coverage by 0.12%.
Report is 1 commits behind head on main.
The diff coverage is 83.87%.

@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
+ Coverage   78.82%   78.95%   +0.12%     
==========================================
  Files          27       28       +1     
  Lines        1176     1207      +31     
==========================================
+ Hits          927      953      +26     
- Misses        249      254       +5     

Files Changed	Coverage Δ
...cesses_dask/process_implementations/cubes/utils.py	86.66% <ø> (ø)
...neo_processes_dask/process_implementations/math.py	59.45% <0.00%> (-0.41%)	⬇️
...ses_dask/process_implementations/cubes/resample.py	86.66% <86.66%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[LukeWeidenwalker]

Just to leave it here too: Since opendatacube/odc-geo#88 came out, it would probably be good to see whether this already does this!

[SerRichard]

Just to leave it here too: Since opendatacube/odc-geo#88 came out, it would probably be good to see whether this already does this!

Switched implementation to use the odc.reproject function, much less code to maintain now!

@ValentinaHutter One thing I'm not sure on. Odc reproject will automatically resample the raster to the provided projection. In the case a user provides no resolution to resample, and only wants a reproject, should we force the resolution of the input cube? Or just use accept resolution as none? Not sure what the preference would be.

[SerRichard]

@ValentinaHutter One other concern, we've no way or reliably enforcing the convention of the user specifying the target resolution in the units of the output crs. Scao recommended this could be enforced with accepted rangers for different CRsystems. Example, if the user incorrectly specifies 0.0009 as their output resolution, and the crs is in metres, this will run for longer. Not sure how much longer on dask, so may not be a huge concern.

[ValentinaHutter]

LGTM!

[ValentinaHutter]

Just to leave it here too: Since opendatacube/odc-geo#88 came out, it would probably be good to see whether this already does this!

Switched implementation to use the odc.reproject function, much less code to maintain now!

@ValentinaHutter One thing I'm not sure on. Odc reproject will automatically resample the raster to the provided projection. In the case a user provides no resolution to resample, and only wants a reproject, should we force the resolution of the input cube? Or just use accept resolution as none? Not sure what the preference would be.

In https://processes.openeo.org/#resample_spatial the default resolution=0 and says that it Doesn't change the resolution by default (0). So, I think this means, we need to keep the input resolution?

[ValentinaHutter]

@ValentinaHutter One other concern, we've no way or reliably enforcing the convention of the user specifying the target resolution in the units of the output crs. Scao recommended this could be enforced with accepted rangers for different CRsystems. Example, if the user incorrectly specifies 0.0009 as their output resolution, and the crs is in metres, this will run for longer. Not sure how much longer on dask, so may not be a huge concern.

Hm, it sounds quite complicated to check this with different ranges for every crs, what do you think? Also, the definition says, that the resolution is Specified in the units of the target projection. So, the user should be aware of this, and we can probably assume, that it is in the required unit. Does data.odc.reproject also assume, that the unit of the resolution matches the output crs?

[SerRichard]

Using odc.geo 0.4.1, I was unable to reproduce what we saw last month one both fake and real (sen2like output) data. Fixed odc minimum version to 0.4.1, fixed tests and accessor import for odc, and reviewed output arrays with @LukeWeidenwalker!

[LukeWeidenwalker]

Looking good! Couple of minor comments, but once these are resolved we should be able to finally merge this!

[LukeWeidenwalker]

Ahh, two more things:

• the spec allows the align parameter, we should at least accept that and log a warning that it doesn't do anything currently edit: we'll just remove this from the spec since we don't support it instead!
• the spec needs to be updated in openeo-processes

[SerRichard]

spec added in eodcgmbh/openeo-processes

[clausmichele]

@SerRichard I have a question: after putting so much work into this process implementation, did you ever test it with an openEO process graph? Because currently it is not imported here https://github.com/Open-EO/openeo-processes-dask/blob/main/openeo_processes_dask/process_implementations/cubes/__init__.py and therefore can't be used.

[clausmichele]

(Edit: my supposition was wrong, see comments below) Additionally, not exposing a parameter like align just because it's not supported it's a bad idea in my opinion.
First of all since it's a parameter with a default value, which could be added by default added to the process graph by the clients (Python client adds it).

@soxofaan does the Python client always add to the process graph all the optional parameters with default values?

[soxofaan]

@soxofaan does the Python client always add to the process graph all the optional parameters with default values?

no we don't always automatically add the default value of unset parameters, it depends a bit on details of DataCube method.

In case of resample_spatial however, we do always add the align parameter with default value "upper-left"

[m-mohr]

Additionally, not exposing a parameter like align just because it's not supported it's a bad idea in my opinion.

Actually, it's the opposite. It's meant to be that if something is not supported, it's not exposed. Process definitions should reflect the implementation status.

In case of resample_spatial however, we do always add the align parameter with default value "upper-left"

I think this should be changed. It should only be sent if not the default value and supported by the back-end.

[clausmichele]

Thanks @m-mohr, so as you said, the Python Client needs to be adjusted.

[soxofaan]

-> Open-EO/openeo-python-client#500

[jdries]

IMO, in case of an argument like 'align', where a backend may not support multiple options but does support one of them, why would it not be good to put this in the backend process spec? Listing the single-value in the enum would be a way to convey important information in a standardized manner, and can solve the issue with defaults.

[m-mohr]

Yes, you are right. If one value is supported, the backend should change the schema accordingly so that it requires exactly that value. If a parameter is not supported at all, it should be removed.

That still would require some kind of change in the Python client, I think.