-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
CLN: clean color selection in _matplotlib/style #37203
Changes from 9 commits
0f1b99a
901453a
201b25f
8e13df5
37a820d
3883a13
f93743c
b4c3267
6af1543
31125f7
45647a4
393ae46
fe66213
1626108
79b0f08
f513bdb
76f7663
0f0f4bc
b8daf79
37734e8
765836f
4479e37
dd9efd7
f0ea701
dedd0dd
b369834
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,81 +1,163 @@ | ||||||
# being a bit too dynamic | ||||||
from typing import ( | ||||||
TYPE_CHECKING, | ||||||
Collection, | ||||||
Dict, | ||||||
Iterable, | ||||||
List, | ||||||
Optional, | ||||||
Sequence, | ||||||
Union, | ||||||
) | ||||||
import warnings | ||||||
|
||||||
import matplotlib.cm as cm | ||||||
import matplotlib.colors | ||||||
import numpy as np | ||||||
|
||||||
from pandas.core.dtypes.common import is_list_like | ||||||
|
||||||
import pandas.core.common as com | ||||||
|
||||||
if TYPE_CHECKING: | ||||||
from matplotlib.colors import Colormap | ||||||
|
||||||
|
||||||
Color = Union[str, Sequence[float]] | ||||||
|
||||||
|
||||||
def get_standard_colors( | ||||||
num_colors: int, colormap=None, color_type: str = "default", color=None | ||||||
num_colors: int, | ||||||
colormap=None, | ||||||
color_type: str = "default", | ||||||
color=None, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also annotate this? |
||||||
): | ||||||
import matplotlib.pyplot as plt | ||||||
|
||||||
colors = _get_colors( | ||||||
color=color, | ||||||
colormap=colormap, | ||||||
color_type=color_type, | ||||||
num_colors=num_colors, | ||||||
) | ||||||
|
||||||
if isinstance(colors, dict): | ||||||
return colors | ||||||
|
||||||
return _cycle_colors(list(colors), num_colors=num_colors) | ||||||
|
||||||
|
||||||
def _get_colors( | ||||||
*, | ||||||
jreback marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
color: Optional[Union[Color, Dict[str, Color], Collection[Color]]], | ||||||
colormap: Optional[Union[str, "Colormap"]], | ||||||
color_type: str, | ||||||
num_colors: int, | ||||||
) -> Union[Dict[str, Color], Collection[Color]]: | ||||||
"""Get colors from user input.""" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a doc-string. also the current summary is not very descriptive, nor is the function name . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed it into |
||||||
if color is None and colormap is not None: | ||||||
if isinstance(colormap, str): | ||||||
cmap = colormap | ||||||
colormap = cm.get_cmap(colormap) | ||||||
if colormap is None: | ||||||
raise ValueError(f"Colormap {cmap} is not recognized") | ||||||
colors = [colormap(num) for num in np.linspace(0, 1, num=num_colors)] | ||||||
return _get_colors_from_colormap(colormap, num_colors=num_colors) | ||||||
elif color is not None: | ||||||
if colormap is not None: | ||||||
warnings.warn( | ||||||
"'color' and 'colormap' cannot be used simultaneously. Using 'color'" | ||||||
) | ||||||
colors = ( | ||||||
list(color) | ||||||
if is_list_like(color) and not isinstance(color, dict) | ||||||
else color | ||||||
) | ||||||
return _get_colors_from_color(color) | ||||||
else: | ||||||
if color_type == "default": | ||||||
# need to call list() on the result to copy so we don't | ||||||
# modify the global rcParams below | ||||||
try: | ||||||
colors = [c["color"] for c in list(plt.rcParams["axes.prop_cycle"])] | ||||||
except KeyError: | ||||||
colors = list(plt.rcParams.get("axes.color_cycle", list("bgrcmyk"))) | ||||||
if isinstance(colors, str): | ||||||
colors = list(colors) | ||||||
|
||||||
colors = colors[0:num_colors] | ||||||
elif color_type == "random": | ||||||
|
||||||
def random_color(column): | ||||||
""" Returns a random color represented as a list of length 3""" | ||||||
# GH17525 use common._random_state to avoid resetting the seed | ||||||
rs = com.random_state(column) | ||||||
return rs.rand(3).tolist() | ||||||
|
||||||
colors = [random_color(num) for num in range(num_colors)] | ||||||
else: | ||||||
raise ValueError("color_type must be either 'default' or 'random'") | ||||||
return _get_colors_from_color_type(color_type, num_colors=num_colors) | ||||||
|
||||||
|
||||||
if isinstance(colors, str) and _is_single_color(colors): | ||||||
# GH #36972 | ||||||
colors = [colors] | ||||||
def _cycle_colors(colors: List[Color], num_colors: int) -> List[Color]: | ||||||
jreback marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
"""Append more colors by cycling if there is not enough color. | ||||||
|
||||||
# Append more colors by cycling if there is not enough color. | ||||||
# Extra colors will be ignored by matplotlib if there are more colors | ||||||
# than needed and nothing needs to be done here. | ||||||
Extra colors will be ignored by matplotlib if there are more colors | ||||||
than needed and nothing needs to be done here. | ||||||
""" | ||||||
if len(colors) < num_colors: | ||||||
try: | ||||||
multiple = num_colors // len(colors) - 1 | ||||||
except ZeroDivisionError: | ||||||
raise ValueError("Invalid color argument: ''") | ||||||
multiple = num_colors // len(colors) - 1 | ||||||
mod = num_colors % len(colors) | ||||||
|
||||||
colors += multiple * colors | ||||||
colors += colors[:mod] | ||||||
|
||||||
return colors | ||||||
|
||||||
|
||||||
def _get_colors_from_colormap( | ||||||
colormap: Union[str, "Colormap"], | ||||||
num_colors: int, | ||||||
) -> Collection[Color]: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. emm, i have seen you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can use Collection for arguments to be permissive where appropriate, but return type should be more precise, in this case List. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, will adjust for return types. I remember exactly @simonjayhawkins suggesting to use more generic types. Thus, I used collection. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep. that's correct for function arguments. return types should be precise. |
||||||
"""Get colors from colormap.""" | ||||||
colormap = _get_cmap_instance(colormap) | ||||||
return [colormap(num) for num in np.linspace(0, 1, num=num_colors)] | ||||||
|
||||||
|
||||||
def _get_cmap_instance(colormap: Union[str, "Colormap"]) -> "Colormap": | ||||||
"""Get instance of matplotlib colormap.""" | ||||||
if isinstance(colormap, str): | ||||||
cmap = colormap | ||||||
colormap = cm.get_cmap(colormap) | ||||||
if colormap is None: | ||||||
raise ValueError(f"Colormap {cmap} is not recognized") | ||||||
return colormap | ||||||
|
||||||
|
||||||
def _get_colors_from_color( | ||||||
color: Union[Color, Dict[str, Color], Collection[Color]], | ||||||
) -> Union[Dict[str, Color], Collection[Color]]: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe change |
||||||
"""Get colors from user input color.""" | ||||||
if isinstance(color, Iterable) and len(color) == 0: | ||||||
raise ValueError("Invalid color argument: {color}") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need an just to clarify: what was the reason to check ValueError here? seems original error message gives an empty string, while here is an empty iterable? is this on purpose? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this check to avoid try/except ZeroDivisionError (in the original code), which would be happening when color == "". I will make it an f-string, I missed that. |
||||||
|
||||||
if isinstance(color, dict): | ||||||
return color | ||||||
|
||||||
if isinstance(color, str): | ||||||
if _is_single_color(color): | ||||||
# GH #36972 | ||||||
return [color] | ||||||
else: | ||||||
return list(color) | ||||||
|
||||||
# ignoring mypy error here | ||||||
# error: Argument 1 to "list" has incompatible type | ||||||
# "Union[Sequence[float], Collection[Union[str, Sequence[float]]]]"; | ||||||
# expected "Iterable[Union[str, Sequence[float]]]" [arg-type] | ||||||
# A this point color may be sequence of floats or series of colors, | ||||||
# all convertible to list | ||||||
return list(color) # type: ignore [arg-type] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. emm, this seems to be a bit more complex than the original comprehension (IIUC, you are adding a patch to convert single color to list in this, right?) is it possible to also write in a comprehension expression? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original portion of the code was exactly quite dynamic. I decided to make it more explicit. I now understand that I missed one case here (which does not hit in tests). This case is when color is sequence of floats (apparently the internal code calls get_standard_colors only with color being sequence of sequence of floats). I will add this edge case and the corresponding test. Presumably, mypy errors will also be resolved. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly speaking, I kind of liked the original comprehension. The problem is that it was not compatible with typing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the ci failure could be fixed with
Suggested change
but please avoid using ignore instead. |
||||||
|
||||||
|
||||||
def _get_colors_from_color_type(color_type: str, num_colors: int) -> Collection[Color]: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. List should be enough |
||||||
"""Get colors from user input color type.""" | ||||||
if color_type == "default": | ||||||
return _get_default_colors(num_colors) | ||||||
elif color_type == "random": | ||||||
return _get_random_colors(num_colors) | ||||||
else: | ||||||
raise ValueError("color_type must be either 'default' or 'random'") | ||||||
|
||||||
|
||||||
def _get_default_colors(num_colors: int) -> Collection[Color]: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. List should be enough |
||||||
"""Get ``num_colors`` of default colors from matplotlib rc params.""" | ||||||
import matplotlib.pyplot as plt | ||||||
|
||||||
# need to call list() on the result to copy so we don't | ||||||
# modify the global rcParams below | ||||||
try: | ||||||
colors = [c["color"] for c in list(plt.rcParams["axes.prop_cycle"])] | ||||||
except KeyError: | ||||||
colors = list(plt.rcParams.get("axes.color_cycle", list("bgrcmyk"))) | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the original code, seems there is a check of if isinstance(colors, str):
colors = list(colors) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, in this particular function colors will not be a string. This check is effectively carried out in |
||||||
return colors[0:num_colors] | ||||||
|
||||||
|
||||||
def _get_random_colors(num_colors: int) -> Sequence[Sequence[float]]: | ||||||
"""Get ``num_colors`` of random colors.""" | ||||||
return [_random_color(num) for num in range(num_colors)] | ||||||
|
||||||
|
||||||
def _random_color(column: int) -> Sequence[float]: | ||||||
"""Get a random color represented as a list of length 3""" | ||||||
# GH17525 use common._random_state to avoid resetting the seed | ||||||
rs = com.random_state(column) | ||||||
return rs.rand(3).tolist() | ||||||
|
||||||
|
||||||
def _is_single_color(color: str) -> bool: | ||||||
"""Check if ``color`` is a single color. | ||||||
|
||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you are doing the refactor here, maybe can you annotate this as well?