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

Remove binary reducers #258 #259

Merged
merged 1 commit into from
Jun 1, 2021
Merged

Remove binary reducers #258 #259

merged 1 commit into from
Jun 1, 2021

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented May 31, 2021

For discussion: Do we actually need the binary reducers and is someone planning to implement them? If not, we may want to clean-up and remove them as otherwise, we'd also need to provide them for e.g. reduce_spatial.

@m-mohr m-mohr added the question Further information is requested label May 31, 2021
@m-mohr m-mohr added this to the 1.1.0 milestone May 31, 2021
@m-mohr m-mohr linked an issue May 31, 2021 that may be closed by this pull request
@jdries
Copy link
Contributor

jdries commented May 31, 2021

Actually we do support the binary variant in our backend, but do not seem to properly declare it:
https://github.com/Open-EO/openeo-python-driver/blob/master/openeo_driver/ProcessGraphDeserializer.py#L842

In fact, it could very well be that we don't properly support the non-binary variant...
Do we actually need them: not if the backend can auto-detect that an optimization is possible, in case the process (graph) is a commutative + associative reducer.

@m-mohr
Copy link
Member Author

m-mohr commented May 31, 2021

Good to know, I've only looked into the Hub and there was no back-end declaring support for it.

Back-ends can surely optimize if it doesn't influence the result. I think the main use case for the binary variants were UDFs.

This issue is pretty much to discuss whether we think those processes are still good to have or whether we could remove them (having in mind #226 and a potential new process reduce_spatial_binary). So all feedback will be appreciated.

@soxofaan
Copy link
Member

soxofaan commented Jun 1, 2021

Actually we do support the binary variant in our backend, but do not seem to properly declare it:
https://github.com/Open-EO/openeo-python-driver/blob/master/openeo_driver/ProcessGraphDeserializer.py#L842

I don't think we (still) support it actually, as far as I understand, the related code path got lost during some 0.4-1.0 related refactors

in geopyspark driver there is this left-over binary argument, but it nowhere used inside the function, and nowhere used when calling the function:
https://github.com/Open-EO/openeo-geopyspark-driver/blob/85d651605d9a25acfec498c5ab86f1fbbe0858ce/openeogeotrellis/geopysparkdatacube.py#L409-L412

and in the python driver, the binary version is also not used anymore (except for checking not to run UDFs binary-style)

https://github.com/Open-EO/openeo-python-driver/blob/7702b92ddffd2ba3de80bd1dcaac7782b023cd08/openeo_driver/ProcessGraphDeserializer.py#L843-L859

@m-mohr m-mohr linked an issue Jun 1, 2021 that may be closed by this pull request
@m-mohr m-mohr merged commit edbd6d8 into draft Jun 1, 2021
@m-mohr
Copy link
Member Author

m-mohr commented Jun 1, 2021

Removing. Has not been implemented and the sole use-case is UDFs, which itself would likely not lead to good performance as binary UDFs are too fine-grained.

@m-mohr m-mohr deleted the remove-binary branch June 1, 2021 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove binary versions of processes? How to compute the average over spatial dimensions?
3 participants