FIX: Enable str transformations to Color/Interval scales #3688
Conversation
| # Prioritize actual colormaps, e.g. if a colormap named "pow" exists | ||
| if is_registered_colormap(arg): | ||
| return Continuous(arg) | ||
| elif any(arg.startswith(k) for k in self._TRANS_ARGS): |
There was a problem hiding this comment.
Should this be startswith or == ? With the current code, passing "logfoo" for instance would redirect it to trans; is that what we want ? Same comment for other parts where this happens.
There was a problem hiding this comment.
Thanks for the comment. I didn't want to change this since it is similar to how it's performed elsewhere in this module and in other parts of the codebase (_core.scales for example). I reckon the original intention in using startswith was to support "log2", "symlog100", etc. Anyway, passing "logfoo" would fail even prior to this PR (with ValueError: 'logfoo' is not a valid palette name).
There was a problem hiding this comment.
+1, the prefix checks allow the "string parameterized scale transform" spelling but maybe that needs better error checking.
Prior to this PR, strings such as "log", "sqrt", etc. (.scale "magic args") are invalid inputs to Color/Interval variables' scales (#2869). It is possible to define a Continuous(trans="log") scale and achieve continuous transformation functionality to these properties. So the following lines are equivalent but the former is currently erroneous:
This PR adds str transformations support directly to Color/Interval properties. It is resolved after property-specific arguments (i.e.
.scale(color="log")will look for "log" colormap first and will apply "log" transformation only if there's no "log" colormap registered).Fixes #2869 .