Repository: matplotlib/matplotlib
Pull request: Cleanup tripcolor()
## PR Summary

Commits:

- Rewrite `Triangulation.get_from_args_and_kwargs` to make parameter parsing testable; and add tests.
- Warn when passing `triangles` or `mask` alongside a `Triangulation` to `tripcolor()`
- Cleanup logic and documentation of `tripcolor()`
  This issues additional warnings and more precise error messages but does not change behavior.
  The only "semantic" change is that I have discouraged passing *triangles* positionally: `tripcolor(x, y, triangles, C)`
  is a bit odd and IMHO has too many positional arguments.

Closes #22303. Follows up on #10148.


Problem statement:
[Bug]: Args for tripcolor
### Bug summary
Thanks for matplotlib! 
`tripcolor` fails if colors are passed with named argument i.e `c=values` instead of just `values`. The error is not very informative and it seems a dangerous practice to ignore `kwargs` and only use `args`.

```bash
  File "/mnt/session_space/conda_envs/latest39/lib/python3.9/site-packages/matplotlib/tri/tripcolor.py", line 57, in tripcolor
    C = np.asarray(args[0])
IndexError: list index out of range
```

### Code for reproduction

```python
plt.tripcolor(vertices[:,0], vertices[:,1], c=values, triangles=triangles)
```

### Actual outcome


```bash
  File "/mnt/session_space/conda_envs/latest39/lib/python3.9/site-packages/matplotlib/tri/tripcolor.py", line 57, in tripcolor
    C = np.asarray(args[0])
IndexError: list index out of range
```

### Expected outcome

a successful plot.

### Additional information

_No response_

### Operating system

_No response_

### Matplotlib Version

3.5.1

### Matplotlib Backend

_No response_

### Python version

_No response_

### Jupyter version

_No response_

### Installation

_No response_

Hints from issue discussion:
Please provide a complete example. (Also, I don't think `c` is a valid keyword argument to `tripcolor`.)
I'll close for lack of response, but do request a re-open if there is still an issue here.  
Basically `tripcolor(np.random.rand(5), np.random.rand(5), C=np.random.rand(5))` currently fails with the not-so-informative error message above (the correct invocation is `tripcolor(np.random.rand(5), np.random.rand(5), np.random.rand(5))`).  I think we should indeed *not* allow passing C by keyword, only positionally (see https://github.com/matplotlib/matplotlib/issues/11526 for essentially the same discussion on quiver); however the error could be made clearer.
Unfortunately, the API is heavily overloaded. We need an `*args` parameter that can take many forms, and we cannot get rid of it without major API breakage. Things to do:

- Clarify which APIs we want to support
- Improve the documentation
- Cleanup the parameter parsing. There seem some assumptions to be baked in. Probably we should have an internal parser function `triangulation, C, kwargs = parse_params(args, kwargs)` that we can unittest the defined API against, and that issues reasonable warnings.

Commit message: Warn when passing triangles or mask alongside a Triangulation

Patch to review:
diff --git a/lib/matplotlib/tests/test_triangulation.py b/lib/matplotlib/tests/test_triangulation.py
index 08ed058ddb2e..dc7292696489 100644
--- a/lib/matplotlib/tests/test_triangulation.py
+++ b/lib/matplotlib/tests/test_triangulation.py
@@ -12,6 +12,52 @@
 from matplotlib.testing.decorators import image_comparison, check_figures_equal
 
 
+x = [-1, 0, 1, 0]
+y = [0, -1, 0, 1]
+triangles = [[0, 1, 2], [0, 2, 3]]
+mask = [False, True]
+
+
+@pytest.mark.parametrize('args, kwargs, expected', [
+    ([x, y], {}, [x, y, None, None]),
+    ([x, y, triangles], {}, [x, y, triangles, None]),
+    ([x, y], dict(triangles=triangles), [x, y, triangles, None]),
+    ([x, y], dict(mask=mask), [x, y, None, mask]),
+    ([x, y, triangles], dict(mask=mask), [x, y, triangles, mask]),
+    ([x, y], dict(triangles=triangles, mask=mask), [x, y, triangles, mask]),
+])
+def test_extract_triangulation_params(args, kwargs, expected):
+    other_args = [1, 2]
+    other_kwargs = {'a': 3, 'b': '4'}
+    x_, y_, triangles_, mask_, args_, kwargs_ = \
+        mtri.Triangulation._extract_triangulation_params(
+            args + other_args, {**kwargs, **other_kwargs})
+    x, y, triangles, mask = expected
+    assert x_ is x
+    assert y_ is y
+    assert_array_equal(triangles_, triangles)
+    assert mask_ is mask
+    assert args_ == other_args
+    assert kwargs_ == other_kwargs
+
+
+def test_extract_triangulation_positional_mask():
+    global x, y, triangles, mask
+    # mask cannot be passed positionally
+    x_, y_, triangles_, mask_, args_, kwargs_ = \
+        mtri.Triangulation._extract_triangulation_params(x, y, triangles, mask)
+    assert mask_ is None
+    assert args_ == [mask]
+    # the positional mask has to be catched downstream because this must pass
+    # unknown args through
+
+
+del x
+del y
+del triangles
+del mask
+
+
 def test_delaunay():
     # No duplicate points, regular grid.
     nx = 5
diff --git a/lib/matplotlib/tri/triangulation.py b/lib/matplotlib/tri/triangulation.py
index 24e99634a466..d9c81de32c6c 100644
--- a/lib/matplotlib/tri/triangulation.py
+++ b/lib/matplotlib/tri/triangulation.py
@@ -1,5 +1,7 @@
 import numpy as np
 
+from matplotlib import _api
+
 
 class Triangulation:
     """
@@ -135,35 +137,43 @@ def get_from_args_and_kwargs(*args, **kwargs):
         """
         if isinstance(args[0], Triangulation):
             triangulation, *args = args
+            if 'triangles' in kwargs:
+                _api.warn_external(
+                    "Passing the keyword 'triangles' has no effect when also "
+                    "passing a Triangulation")
+            if 'mask' in kwargs:
+                _api.warn_external(
+                    "Passing the keyword 'mask' has no effect when also "
+                    "passing a Triangulation")
         else:
-            x, y, *args = args
-
-            # Check triangles in kwargs then args.
-            triangles = kwargs.pop('triangles', None)
-            from_args = False
-            if triangles is None and args:
-                triangles = args[0]
-                from_args = True
-
-            if triangles is not None:
-                try:
-                    triangles = np.asarray(triangles, dtype=np.int32)
-                except ValueError:
-                    triangles = None
-
-            if triangles is not None and (triangles.ndim != 2 or
-                                          triangles.shape[1] != 3):
-                triangles = None
-
-            if triangles is not None and from_args:
-                args = args[1:]  # Consumed first item in args.
-
-            # Check for mask in kwargs.
-            mask = kwargs.pop('mask', None)
-
+            x, y, triangles, mask, args, kwargs = \
+                Triangulation._extract_triangulation_params(args, kwargs)
             triangulation = Triangulation(x, y, triangles, mask)
         return triangulation, args, kwargs
 
+    @staticmethod
+    def _extract_triangulation_params(args, kwargs):
+        x, y, *args = args
+        # Check triangles in kwargs then args.
+        triangles = kwargs.pop('triangles', None)
+        from_args = False
+        if triangles is None and args:
+            triangles = args[0]
+            from_args = True
+        if triangles is not None:
+            try:
+                triangles = np.asarray(triangles, dtype=np.int32)
+            except ValueError:
+                triangles = None
+        if triangles is not None and (triangles.ndim != 2 or
+                                      triangles.shape[1] != 3):
+            triangles = None
+        if triangles is not None and from_args:
+            args = args[1:]  # Consumed first item in args.
+        # Check for mask in kwargs.
+        mask = kwargs.pop('mask', None)
+        return x, y, triangles, mask, args, kwargs
+
     def get_trifinder(self):
         """
         Return the default `matplotlib.tri.TriFinder` of this
diff --git a/lib/matplotlib/tri/tripcolor.py b/lib/matplotlib/tri/tripcolor.py
index f1f7de2285dc..8fc21b5b7c48 100644
--- a/lib/matplotlib/tri/tripcolor.py
+++ b/lib/matplotlib/tri/tripcolor.py
@@ -11,22 +11,24 @@ def tripcolor(ax, *args, alpha=1.0, norm=None, cmap=None, vmin=None,
     """
     Create a pseudocolor plot of an unstructured triangular grid.
 
-    The triangulation can be specified in one of two ways; either::
+    Call signatures::
 
-      tripcolor(triangulation, ...)
+      tripcolor(triangulation, C, *, ...)
+      tripcolor(x, y, C, *, [triangles=triangles], [mask=mask], ...)
 
-    where triangulation is a `.Triangulation` object, or
+    The triangular grid can be specified either by passing a `.Triangulation`
+    object as the first parameter, or by passing the points *x*, *y* and
+    optionally the *triangles* and a *mask*. See `.Triangulation` for an
+    explanation of these parameters.
 
-    ::
+    Parameters
+    ----------
+    triangulation : `.Triangulation`
+        An already created triangular grid.
+    x, y, triangles, mask
+        Parameters specifying defining the triangular grid. See
+        `.Triangulation`.
 
-      tripcolor(x, y, ...)
-      tripcolor(x, y, triangles, ...)
-      tripcolor(x, y, triangles=triangles, ...)
-      tripcolor(x, y, mask=mask, ...)
-      tripcolor(x, y, triangles, mask=mask, ...)
-
-    in which case a Triangulation object will be created.  See `.Triangulation`
-    for a explanation of these possibilities.
 
     The next argument must be *C*, the array of color values, either
     one per point in the triangulation if color values are defined at
@@ -42,6 +44,11 @@ def tripcolor(ax, *args, alpha=1.0, norm=None, cmap=None, vmin=None,
     three points. If *shading* is 'gouraud' then color values must be
     defined at points.
 
+
+
+    tripcolor(x, y, [triangles], C, [mask=mask], ...)
+
+
     The remaining kwargs are the same as for `~.Axes.pcolor`.
     """
     _api.check_in_list(['flat', 'gouraud'], shading=shading)


Code review comments:
diff_hunk: @@ -12,6 +12,73 @@
 from matplotlib.testing.decorators import image_comparison, check_figures_equal
 
 
+x = [-1, 0, 1, 0]
+y = [0, -1, 0, 1]
+triangles = [[0, 1, 2], [0, 2, 3]]
+mask = [False, True]
path: lib/matplotlib/tests/test_triangulation.py
text: @user1:
Not sure I like having these globals here. You can hide them in a parametrized fixture, I think.

@author:
I agree that they are not quite nice, but were the best compromise I found to keep the following parametrization readable.

```
@user2.mark.parametrize('args, kwargs, expected', [
    ([x, y], {}, [x, y, None, None]),
    ([x, y, triangles], {}, [x, y, triangles, None]),
    ([x, y], dict(triangles=triangles), [x, y, triangles, None]),
    ([x, y], dict(mask=mask), [x, y, None, mask]),
    ([x, y, triangles], dict(mask=mask), [x, y, triangles, mask]),
    ([x, y], dict(triangles=triangles, mask=mask), [x, y, triangles, mask]),
])
def test_extract_triangulation_params(args, kwargs, expected):
```

> You can hide them in a parametrized fixture, I think.

Can you please explain what you mean by that? I could hard-code them in the above snippet, but I'm afraid then nobody will be able to understand what this does. Any improvement is welcome.


@user1:
One way to write it is something like this:
```python
@user2.mark.parametrize('arg_set', range(6))
def test_extract_triangulation_params(arg_set):
    x = [-1, 0, 1, 0]
    y = [0, -1, 0, 1]
    triangles = [[0, 1, 2], [0, 2, 3]]
    mask = [False, True]

    arg, kwargs, expected = [
        ([x, y], {}, [x, y, None, None]),
        ([x, y, triangles], {}, [x, y, triangles, None]),
        ([x, y], dict(triangles=triangles), [x, y, triangles, None]),
        ([x, y], dict(mask=mask), [x, y, None, mask]),
        ([x, y, triangles], dict(mask=mask), [x, y, triangles, mask]),
        ([x, y], dict(triangles=triangles, mask=mask), [x, y, triangles, mask]),
    ][arg_set]
```
or perhaps better might be to hide it in a class?
```python
class TestTriangulationParams:
    x = [-1, 0, 1, 0]
    y = [0, -1, 0, 1]
    triangles = [[0, 1, 2], [0, 2, 3]]
    mask = [False, True]

    @user2.mark.parametrize('args, kwargs, expected', [
        ([x, y], {}, [x, y, None, None]),
        ([x, y, triangles], {}, [x, y, triangles, None]),
        ([x, y], dict(triangles=triangles), [x, y, triangles, None]),
        ([x, y], dict(mask=mask), [x, y, None, mask]),
        ([x, y, triangles], dict(mask=mask), [x, y, triangles, mask]),
        ([x, y], dict(triangles=triangles, mask=mask), [x, y, triangles, mask]),
    ])
    def test_extract_triangulation_params(self, args, kwargs, expected):
        ...
```

@author:
The class is a good idea.

diff_hunk: @@ -11,65 +11,93 @@ def tripcolor(ax, *args, alpha=1.0, norm=None, cmap=None, vmin=None,
     """
     Create a pseudocolor plot of an unstructured triangular grid.
 
-    The triangulation can be specified in one of two ways; either::
-
-      tripcolor(triangulation, ...)
-
-    where triangulation is a `.Triangulation` object, or
-
-    ::
-
-      tripcolor(x, y, ...)
-      tripcolor(x, y, triangles, ...)
-      tripcolor(x, y, triangles=triangles, ...)
-      tripcolor(x, y, mask=mask, ...)
-      tripcolor(x, y, triangles, mask=mask, ...)
-
-    in which case a Triangulation object will be created.  See `.Triangulation`
-    for a explanation of these possibilities.
-
-    The next argument must be *C*, the array of color values, either
-    one per point in the triangulation if color values are defined at
-    points, or one per triangle in the triangulation if color values
-    are defined at triangles. If there are the same number of points
-    and triangles in the triangulation it is assumed that color
-    values are defined at points; to force the use of color values at
-    triangles use the kwarg ``facecolors=C`` instead of just ``C``.
-
-    *shading* may be 'flat' (the default) or 'gouraud'. If *shading*
-    is 'flat' and C values are defined at points, the color values
-    used for each triangle are from the mean C of the triangle's
-    three points. If *shading* is 'gouraud' then color values must be
-    defined at points.
-
-    The remaining kwargs are the same as for `~.Axes.pcolor`.
+    Call signatures::
+
+      tripcolor(triangulation, C, *, ...)
+      tripcolor(x, y, C, *, [triangles=triangles], [mask=mask], ...)
+
+    The triangular grid can be specified either by passing a `.Triangulation`
+    object as the first parameter, or by passing the points *x*, *y* and
+    optionally the *triangles* and a *mask*. See `.Triangulation` for an
+    explanation of these parameters.
+
+    If neither of *triangulation* or *triangles* are given, the triangulation
+    is calculated on the fly. In this case, it does not make sense to provide
+    colors at the triangle faces via *C* or *facecolors* because there are
+    multiple possible triangulations for a group of points and you don't know
+    which triangles will be constructed.
+
+    Parameters
+    ----------
+    triangulation : `.Triangulation`
+        An already created triangular grid.
+    x, y, triangles, mask
+        Parameters specifying defining the triangular grid. See
+        `.Triangulation`. This is mutually exclusive with specifying
+        *triangulation*.
+    C : array-like
+        The color values, either for the points or for the triangles. Which one
+        is automatically inferred from the length of *C*, i.e. does it match
+        the number of points or the number of triangles. If there are the same
+        number of points and triangles in the triangulation it is assumed that
+        color values are defined at points; to force the use of color values at
+        triangles use the keyword argument ``facecolors=C`` instead of just
+        ``C``.
+        This parameter is position-only.
+    facecolors : array-like, optional
+        Can be used alternatively to *C* to specify colors at the triangle
+        faces. This parameter takes precedence over *C*.
+    shading : {'flat', 'gouraud'}, default: 'flat'
+        If is 'flat' and the color values *C* are defined at points, the color
path: lib/matplotlib/tri/tripcolor.py
text: @user1:
```suggestion
        If 'flat' and the color values *C* are defined at points, the color
```

diff_hunk: @@ -56,13 +60,25 @@ def __init__(self, x, y, triangles=None, mask=None):
         else:
             # Triangulation specified. Copy, since we may correct triangle
             # orientation.
-            self.triangles = np.array(triangles, dtype=np.int32, order='C')
+            try:
+                self.triangles = np.array(triangles, dtype=np.int32, order='C')
+            except ValueError as e:
+                raise ValueError('triangles must be a (N, 3) int array, not '
+                                 f'{triangles!r}') from e
             if self.triangles.ndim != 2 or self.triangles.shape[1] != 3:
-                raise ValueError('triangles must be a (?, 3) array')
+                raise ValueError(
+                    'triangles must be a (N, 3) array, but found shape '
path: lib/matplotlib/tri/triangulation.py
text: @user1:
Keep `int` in here to match above?
```suggestion
                    'triangles must be a (N, 3) int array, but found shape '
```

diff_hunk: @@ -105,15 +131,17 @@ def tripcolor(ax, *args, alpha=1.0, norm=None, cmap=None, vmin=None,
         # Color values.
         if facecolors is None:
             # One color per triangle, the mean of the 3 vertex color values.
-            C = C[maskedTris].mean(axis=1)
+            colors = point_colors[maskedTris].mean(axis=1)
         elif tri.mask is not None:
             # Remove color values of masked triangles.
-            C = C[~tri.mask]
+            colors = facecolors[~tri.mask]
+        else:
+            colors = facecolors
 
         collection = PolyCollection(verts, **kwargs)
 
     collection.set_alpha(alpha)
-    collection.set_array(C)
+    collection.set_array(colors)
path: lib/matplotlib/tri/tripcolor.py
text: @user1:
While you're at it, probably kwargs should be set last, for the same reason as in #21146/#21525?

@author:
But there is `collection._scale_norm(norm, vmin, vmax)` after setting the norm here. AFAICS this handles the issue in #21146/#21525. One can probably remove that when delaying kwargs setting. But since this PR is already complicated enough and there is no immediate problem, I'd not go into this within this PR.

diff_hunk: @@ -11,65 +11,93 @@ def tripcolor(ax, *args, alpha=1.0, norm=None, cmap=None, vmin=None,
     """
     Create a pseudocolor plot of an unstructured triangular grid.
 
-    The triangulation can be specified in one of two ways; either::
-
-      tripcolor(triangulation, ...)
-
-    where triangulation is a `.Triangulation` object, or
-
-    ::
-
-      tripcolor(x, y, ...)
-      tripcolor(x, y, triangles, ...)
-      tripcolor(x, y, triangles=triangles, ...)
-      tripcolor(x, y, mask=mask, ...)
-      tripcolor(x, y, triangles, mask=mask, ...)
-
-    in which case a Triangulation object will be created.  See `.Triangulation`
-    for a explanation of these possibilities.
-
-    The next argument must be *C*, the array of color values, either
-    one per point in the triangulation if color values are defined at
-    points, or one per triangle in the triangulation if color values
-    are defined at triangles. If there are the same number of points
-    and triangles in the triangulation it is assumed that color
-    values are defined at points; to force the use of color values at
-    triangles use the kwarg ``facecolors=C`` instead of just ``C``.
-
-    *shading* may be 'flat' (the default) or 'gouraud'. If *shading*
-    is 'flat' and C values are defined at points, the color values
-    used for each triangle are from the mean C of the triangle's
-    three points. If *shading* is 'gouraud' then color values must be
-    defined at points.
-
-    The remaining kwargs are the same as for `~.Axes.pcolor`.
+    Call signatures::
+
+      tripcolor(triangulation, C, *, ...)
+      tripcolor(x, y, C, *, [triangles=triangles], [mask=mask], ...)
+
+    The triangular grid can be specified either by passing a `.Triangulation`
+    object as the first parameter, or by passing the points *x*, *y* and
+    optionally the *triangles* and a *mask*. See `.Triangulation` for an
+    explanation of these parameters.
+
+    If neither of *triangulation* or *triangles* are given, the triangulation
+    is calculated on the fly. In this case, it does not make sense to provide
+    colors at the triangle faces via *C* or *facecolors* because there are
+    multiple possible triangulations for a group of points and you don't know
+    which triangles will be constructed.
+
+    Parameters
+    ----------
+    triangulation : `.Triangulation`
+        An already created triangular grid.
+    x, y, triangles, mask
+        Parameters specifying defining the triangular grid. See
path: lib/matplotlib/tri/tripcolor.py
text: @user1:
specifying defining?

diff_hunk: @@ -12,6 +12,73 @@
 from matplotlib.testing.decorators import image_comparison, check_figures_equal
 
 
+x = [-1, 0, 1, 0]
+y = [0, -1, 0, 1]
+triangles = [[0, 1, 2], [0, 2, 3]]
+mask = [False, True]
+
+
+@pytest.mark.parametrize('args, kwargs, expected', [
+    ([x, y], {}, [x, y, None, None]),
+    ([x, y, triangles], {}, [x, y, triangles, None]),
+    ([x, y], dict(triangles=triangles), [x, y, triangles, None]),
+    ([x, y], dict(mask=mask), [x, y, None, mask]),
+    ([x, y, triangles], dict(mask=mask), [x, y, triangles, mask]),
+    ([x, y], dict(triangles=triangles, mask=mask), [x, y, triangles, mask]),
+])
+def test_extract_triangulation_params(args, kwargs, expected):
+    other_args = [1, 2]
+    other_kwargs = {'a': 3, 'b': '4'}
+    x_, y_, triangles_, mask_, args_, kwargs_ = \
+        mtri.Triangulation._extract_triangulation_params(
+            args + other_args, {**kwargs, **other_kwargs})
+    x, y, triangles, mask = expected
+    assert x_ is x
+    assert y_ is y
+    assert_array_equal(triangles_, triangles)
+    assert mask_ is mask
+    assert args_ == other_args
+    assert kwargs_ == other_kwargs
+
+
+del x
+del y
+del triangles
+del mask
+
+
+def test_extract_triangulation_positional_mask():
+    # mask cannot be passed positionally
+    mask = [True]
+    args = [[0, 2, 1], [0, 0, 1], [[0, 1, 2]], mask]
+    x_, y_, triangles_, mask_, args_, kwargs_ = \
+        mtri.Triangulation._extract_triangulation_params(args, {})
+    assert mask_ is None
+    assert args_ == [mask]
+    # the positional mask has to be catched downstream because this must pass
path: lib/matplotlib/tests/test_triangulation.py
text: @user1:
```suggestion
    # the positional mask must be caught downstream because this must pass
```