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

fix: don't rename the output names for multi output reducers #435

Merged
merged 3 commits into from
Feb 24, 2025
Merged

Conversation

12rambau
Copy link
Member

@12rambau 12rambau commented Feb 21, 2025

Fix #432

overview

This is trying to solve the multi-output reducer problem without breaking everything that already exists. The issues linked to this PR shows that we went one step to far in this function by renaming all the bands in the image as if the reducer used to aggregate the images has multiple outputs then .... it's obviously impossible to keep only the band name.
Earth engine is enoying for this matter because he behaves differently depending on the reducer image combination:

  • single band/single output: the output has the same name as the band
  • every other cobination: <band_name>_<reducer_name>

What I decided to set up as a default behaviour in our reduceIntervalmethod is : whatever the number of band or reducer the outputs will always be named <band_name>_<reducer_name>.

So I included the following piece of code outside the reducer to make an inventory of the band/reducer combination:

def outputNames(b):
            return red.getOutputs().map(lambda o: ee.String(b).cat("_").cat(o))

        bandNames = self._obj.first().bandNames().map(outputNames).flatten()

Open question how to avoid breaking everything that is already existing ?
My solution is to set a new temporary parameter in the function keep_original_names that enforce the previous behaviour (rename everything as the bands). This is triggered only when the outputs are single and will systematically send a warning to the user.

        if keep_original_names is True and red.getOutputs().length().getInfo() == 1:
            msg = (
                "The `keep_original_names` parameter will be removed in future versions\n"
                " and band names will not be renamed anymore. To keep the old behaviour, rename manually\n"
                " the output like `ic = ic.map(lambda i: i.rename([<list of the original bands>]))`.\n"
                " To drop this warning set the `keep_original_names` parameter to `False`."
            )
            warnings.warn(msg, category=DeprecationWarning, stacklevel=2)
            bandNames = self._obj.first().bandNames()

Long term evolution

  • in few releases we will simply deprecate the renaming behaviour and start sending a warning when the parameter is manually set
  • even further in the future we will be able to drop the parameter all together

side quest

As i was there I regenerated the tests files with the new ee_dictionary_regression fixture, it's easier to read and cost me 0 time.

Copy link
Member

@fitoprincipe fitoprincipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The process itself looks good, I tested with different scenarios and worked every time

@12rambau 12rambau merged commit 48b43e1 into main Feb 24, 2025
11 checks passed
@12rambau 12rambau deleted the reducers branch February 24, 2025 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multi output reducer: geetools.reduceInterval with reducer =minMax not working as expected
2 participants