Open-EO / openeo-python-client

Python client API for OpenEO
https://open-eo.github.io/openeo-python-client/
Apache License 2.0
156 stars 42 forks source link

Prevent "band math" like attempts with `filter_bands` #239

Open soxofaan opened 3 years ago

soxofaan commented 3 years ago

Spin-off from https://github.com/Open-EO/openeo-python-client/issues/76#issuecomment-923896434 :

User might mistakenly use filter_bands() instead of bands() to do band math: e.g.

green = cube.filter_bands("green")
nir = cube.filter_bands("nir")
ndwi = (green - nir) / (green + nir)

This will currently be translated to 3 merge_cubes operations with an overlap resolver (one for each of the three math operators). Not only is this very inefficient, it will also give unexpected results.

We should raise error or warning if we detect this (doing math operation on filter_bands result)

soxofaan commented 3 years ago

it will also give unexpected results.

If I'm not mistaken, this is what should happen in pseudo code:

final result will be a 4D datacube (2 bands) with 1 values everywhere :0

soxofaan commented 3 years ago

For a bit more context: when the client works with a "math" expression like cube1 + cube2:

The first case works fine and is a handy feature. The second case is more tricky because there is no guarantee that the overlap reducer will be used at all (it depends on the overlap between the cubes), so the results might be not intuitive or hard to explain. For example if the math expression is cube1 - cube2, the output values of the result will be:

Users probably have different expectations: for example: only have cube1-cube2 where there is overlap and leave the rest NaN, or at least have 0 - cube2 where only cube2 is defined.

I think we should reconsider using merge_cubes for implementing math operators in a non-band-math context.

soxofaan commented 3 years ago

FYI as far as I can remember, merge_cubes based math was introduced to have easy to read mask operations like

scene_classification = con.load_collection("SCENECLASSIFICATION", ...)
mask = (scene_classification == 4) | (scene_classification == 5)

which works without issues mentioned above because there is 100% overlap

Some options to resolve this whole issue:

what do you think @jdries ?

soxofaan commented 3 years ago

Having an "overlap-only" version of merge_cubes might also be a solution: see https://github.com/Open-EO/openeo-processes/issues/280

jdries commented 3 years ago

I seem to be mostly in favor of trying to detect this case on the client side. A substraction between two cubes with the same number of bands, and different band names, should not result in a cube with 2 times the number of bands. I think that's at least how the user would expect it to work.

So throwing an error is fine for me. There's possibly also an option to introduce even more logic in the client to resolve the case, but that might be too much magic?