Open-EO / openeo-python-client

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

logic operator support in ProcessBuilder #266

Open soxofaan opened 2 years ago

soxofaan commented 2 years ago

reported by @clausmichele :

I need to recreate this PG, basically doing equal twice and ten combining with OR: ... I've tried ... .reduce_dimension(dimension='bands',reducer=lambda x: x==2 or x==3) but the result is not the same. .... found the solution ... .reduce_dimension(dimension="bands",reducer = lambda value: or_(eq(array_element(value,0),2),eq(array_element(value,0),3)))

my initial response:

the Python or is not supported in this callback building feature. The == operator is supported so this should work too: lambda x: or_(x==2, x==3)

In python it's not possible to overload the or and and "operators" ,
but it is possible to overload their bitwise variants | and & (as done in other libraries like numpy) if I'm not mistaken

danielFlemstrom commented 2 years ago

Finally, I ended up there as well. Is there a way of generating an error message if the client user tries to use and ?

Would it be OK to somehow only use bitwise_and, bitwise_or and bitwise_xor ? I have implemented a first bitwise process implentation to be submitted to openeo-processes-python that accepts most combinations of input.

When "masking bitmasks" we need to use bitwise_and / or. E.g Sentinel3 olci images have a composite bitmask showing whether a pixel is land or sea, or cludy and so on.

soxofaan commented 2 years ago

Is there a way of generating an error message if the client user tries to use and ?

no, I am afraid that is not really feasible here. Because and and or can not be overloaded or intercepted, the openeo client library can not detect that the user is using these.

Note that logical operators on bitmasks are supported and often used already, e.g. with the "band math" feature. For example from https://github.com/Open-EO/openeo-python-client/blob/master/examples/notebooks/openeo-terrascope-webinar.ipynb: Screenshot from 2022-09-01 15-34-56

clausmichele commented 2 years ago

@soxofaan the issue is that there can be multiple S3 OLCI flags active for the same pixel and therefore it's necessary a bitwise operator as explained here https://github.com/Open-EO/openeo-processes-python/issues/179

soxofaan commented 2 years ago

If I understand correctly, that is a completely different issue. The openeo-processes project does not define bitwise and/or processes, only logical and/or. So that discussion should first be taken there.

The original issue here is that a user tries to use and/or Python operators to and expect it to to translate to the openeo processes and and or, which unfortunately is not feasible.

clausmichele commented 2 years ago

If I understand correctly, that is a completely different issue. The openeo-processes project does not define bitwise and/or processes, only logical and/or. So that discussion should first be taken there.

The original issue here is that a user tries to use and/or Python operators to and expect it to to translate to the openeo processes and and or, which unfortunately is not feasible.

You are right sorry, I misinterpreted the example you have given as a possible solution for the S3 bit masks, but instead was just an example of overloaded operators.

danielFlemstrom commented 2 years ago

Yes sorry, maybe i was confusing things here, so to clarify the problem: 1) I guess we all agree on that the overloading of (logical) and, or,xor is not possible. 2) My (second) question is whether we can connect the operators (& | ^) to the bitwise version istead of logical and/or/xor. After all & IS the bitwise or operator... But I have not the complete overview of the consequences of such a soluition... I will do that in our fork and see what happens there, but I will need some assistance for the bigger picture. 3) We need to define processes for the bitwise operators (i am currently working on a first iteration of all this)

soxofaan commented 2 years ago

My (second) question is whether we can connect the operators (& | ^) to the bitwise version istead of logical and/or/xor. After all & IS the bitwise or operator... But I have not the complete overview of the consequences of such a soluition

Indeed, the openEO Python client library currently translates the bitwise Python operators & and | to the logical and/or openEO Processes. I understand this looks not ideal, but:

Currently it would be a step back usability wise to drop that translation for no gain. For example, it is possible to do something like

scl = cube.band("SCENECLASSIFICATION")
mask = (scl == 4) | (scl == 8) | (scl == 9) | (scl == 11)

if we drop that syntactic sugar feature, a user would be forced to write.something like

scl = cube.band("SCENECLASSIFICATION")
mask = (scl == 4).logical_or(scl == 8).logical_or(scl == 9).logical_or(scl == 11)

which looks a bit weird.

And even if bitwise and/or openEO processes are introduced, I'd think that the logical and/or processes will be a lot more common in general usage than the bitwise ones, so one can argue that it's not ideal to make the most common use case usability-wise harder than the least common one.

In any case, if bitwise and/or openEO processes are introduced, the client will support it anyway, at least with a regular method name like bitwise_or (like we have logical_or already as illustrated above). Syntactic sugar (translating & and | operators) is just something nice to have.

We need to define processes for the bitwise operators (i am currently working on a first iteration of all this)

I understand you try to do that in openeo-processes-python . But before you start that, you should open the discussion in the openeo-processes project.

danielFlemstrom commented 2 years ago

Yes I have started this discussion all over the place :) Amazing that such a basic and "simple" thing can be so complicated when you look at it more closely. As usual the devil is in the details, i guess...

Telling the users that and/or/xor is ignored is one thing, but connecting the operators to wrong process is still problematic since it would yield the wrong result in some situations but not in all. At least our "customers" would expect the operators to work as intended. Normally, a bitwise operator should give the same(logical) result as the logical one if you really do booleans or arrays of booleans. What complicates things here is that we have arrays of god-knows-what. But as long as an logical_and is done between two bool or two arrays of bool, yielding an element-wise result, we should be good right? I really do not like the A and B=>B, especially for non booleans, but should you really rely on that ?