Skip to content
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

Prevent "band math" like attempts with filter_bands #239

Open
soxofaan opened this issue Sep 21, 2021 · 5 comments
Open

Prevent "band math" like attempts with filter_bands #239

soxofaan opened this issue Sep 21, 2021 · 5 comments

Comments

@soxofaan
Copy link
Member

Spin-off from #76 (comment) :

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
Copy link
Member Author

soxofaan commented Sep 21, 2021

it will also give unexpected results.

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

  • green - nir -> merge_cubes(cube1="filter_green", cube2= "filter_nir"}, overlap_resolver="subtract") Because band labels are different there is no overlap to be resolved and the result will be 4D datacube with the original green and nir bands -> datacube "diff"
  • green + nir -> same reasoning: result will be 4D datacube with the original green and nir bands -> datacube "sum"
  • diff / sum -> `merge_cubes(cube="diff", cube2="sum", overlap_resolver="divide") , now because "diff" and "sum are actually identical, there is overlap to be resolved by "divide" operation results in always 1 (unless input is zero, then it's NaN)

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

@soxofaan
Copy link
Member Author

soxofaan commented Sep 22, 2021

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

  • if both data cubes come from a .band() operation, "band math" mode is used: reduce_dimension along "bands" dimension with a reducer that implements the operation (add in this case)
  • otherwise (e.g. cubes come from .filter_bands() or something else), merge_cubes is used with an overlap resolver that implements the operation

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:

  • cube1 where only cube1 is defined
  • cube1 - cube2 where there is overlap
  • cube2 where only cube2 is defined

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
Copy link
Member Author

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:

  • only support merge_cubes based math for logical operators (and, or, not), but drop support for standard math operators (+, -, * ...)
  • try to detect in client if overlap is 100%, and throw an error, so that there are no surprises
  • drop merge_cubes based math all together and promote explicit usage of merge_cubes with ProcessBuilder for the overlap resolver

what do you think @jdries ?

@soxofaan
Copy link
Member Author

Having an "overlap-only" version of merge_cubes might also be a solution: see Open-EO/openeo-processes#280

@jdries
Copy link
Collaborator

jdries commented Sep 22, 2021

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants