satpy icon indicating copy to clipboard operation
satpy copied to clipboard

Documentation for colorize seems incomplete and built-in enhancements do not follow documented API

Open gerritholl opened this issue 3 years ago • 1 comments

Describe the bug

The documentation for satpy.enhancements.colorize is unclear and not matched by any of the examples from the enhancements built-in to Satpy. The documentation states that there are four ways to specify the palette:

https://github.com/pytroll/satpy/blob/dbe2838a5ed825a9a57b0a7449ad939159afbc09/satpy/enhancements/init.py#L313-L332

This seems to be incomplete. None of the enhancements built into Satpy use the documented calling signature, and yet they work:

https://github.com/pytroll/satpy/blob/dbe2838a5ed825a9a57b0a7449ad939159afbc09/satpy/etc/enhancements/abi.yaml#L189-L202

uses values, colors, and color_scale. It does not use min_value and max_value despite the documentation indicating they are required for all alternatives, and it uses the undocumented argument color_scale. It also passes a list of lists rather than a tuple of tuples for the colors key, but that is a minor issue as lists and tuples are interchangeable in many cases (as apparently here).

https://github.com/pytroll/satpy/blob/dbe2838a5ed825a9a57b0a7449ad939159afbc09/satpy/etc/enhancements/abi.yaml#L232-L237

passes a string to colors, despite the documentation indicating it must be a trollimage colormap instance or a tuple of tuples. Probably where the documentation indicates a trollimage.colormap.Colormap instance, a string that can be converted to such is also accepted, such as here as well:

https://github.com/pytroll/satpy/blob/dbe2838a5ed825a9a57b0a7449ad939159afbc09/satpy/etc/enhancements/generic.yaml#L775-L777

However, this behaviour is undocumented.

Then there are cases where colors is not a tuple of RGB(A) tuples, but a list of [value, [R, G, B]] pairs, which is also undocumented:

https://github.com/pytroll/satpy/blob/dbe2838a5ed825a9a57b0a7449ad939159afbc09/satpy/etc/enhancements/mimic.yaml#L68-L79

In this case, the values key is missing, despite the documentation suggesting this is mandatory for all alternatives. This also applies here:

https://github.com/pytroll/satpy/blob/dbe2838a5ed825a9a57b0a7449ad939159afbc09/satpy/etc/enhancements/viirs.yaml#L17-L48

There are no built-in enhancements that use the alternative where all of values, min_value, and max_value are passed (4th alternative in documentation).

To Reproduce

  • Read the documentation
  • Look at enhancements
  • Try to match the two

Expected behavior

I expect that the documentation describes completely and comprehensively the valid calling signature for colorize and palettize. This is even more important because I don't think the YAML recipes are covered by unit tests, so if an enhancement would use an undocumented calling signature and this would cease to work, we would not detect it automatically.

Actual results

See above; discrepancy between documentation and built-in enhancements, therefore unclear what is the valid calling signature.

Environment Info:

Latest main branch.

Additional context

I could use inspection in combination with trial and error to see what works and what doesn't work. However, API documentation also serves to define what is accepted and what isn't. Hopefully, documented behaviour is covered by unit tests. Undocumented behaviour may not be covered by unit tests. Unless the YAML recipes themselves are covered by unit tests, which I believe aren't at the moment (at least coverage checks don't require them to be), it is risky to use undocumented behaviour in YAML recipes.

There are other places with documentation on creating colormaps:

  • https://github.com/pytroll/satpy/pull/2075 has added an example to the overall enhancements documentation, available at https://satpy.readthedocs.io/en/stable/enhancements.html#colorize
  • create_colormap is another function that creates a colormap, that seems to be more comprehensively documented than colorize.

Maybe the arguments for the palette passed to colorize or palettize is the same as for create_colormap?

gerritholl avatar Aug 11 '22 16:08 gerritholl

I'm catching up on email after vacation so not sure I understood everything you said, but I'll see what I can answer here. It is a little hard to follow in the code but colorize uses _merge_colormaps to create one "appended"/merged colormap from multiple sub-colormaps. Those sub-colormaps are created using create_colormap. The create_colormap function has grown over time with a lot of new features and may in the future even be absorbed by trollimage (I think all of the functionality is now builtin to the trollimage Colormap class).

Possible solution to your problem: Clean up any examples in the colorize docstring, but point to and emphasize that any colormap description is passed to create_colormap (see that function for possibilities).

As for testing and uses: all possible functionality should be tested and documented (in the docstring) but may not be used in the builtin YAML recipes/configs. We should probably, if our documenting is done well, consider the builtin configs to be something that could go away in the future (not that they will) and should not be used as a source of documentation. The documentation (sphinx + docstrings) should be good enough to stand on their own. That's my opinion at least.

djhoese avatar Aug 13 '22 20:08 djhoese