Surface plots

[johnomotani]

.plot.surface() method that wraps matplotlib's plot_surface() method from the 3d toolkit.

• works with facet grids
• disables all the automatic color-map, etc. because it doesn't necessarily make sense for surface plots - the default (in matplotlib, and as implemented now) is not to color the surface. xarray's auto-selection stuff would interfere with plot_surface()'s kwargs.

I'm not sure if there's somewhere good to note the new surface() method in the docs to make it more discoverable? Maybe in one of the examples?

• Closes plot_surface() wrapper #5084
• Tests added
• Passes pre-commit run --all-files
• User visible changes (including notable bug fixes) are documented in whats-new.rst
• New functions/methods are listed in api.rst

[johnomotani]

Note, this PR includes the bugfix in #5099, because it needed to build on that change. #5099 is smaller, so I guess it will naturally be merged first.

[johnomotani]

Seems like the way I've implemented surface plots requires a slightly newer version than the oldest supported matplotlib to use the "3d" projection. matplotlib-3.2.0 basically works, but some of the unit tests require matpltolib-3.3.0 to pass. I'll try to add a check and skip some tests where necessary.

[pep8speaks]

Hello @johnomotani! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-03 12:06:43 UTC

[johnomotani]

Tests pass! Think this is ready for review now @pydata/xarray

[mathause]

Again, thanks for your patience! I left some comments but I think the implementation looks good overall. I suggest we merge #5099 first and then get this one under way. I would prefer to have the fix and the new feature in two different PRs.

[mathause]

Suggested change

	del LooseVersion
	del mpl

Not necessary.

[johnomotani]

I think these were necessary, because of this a bit later

xarray/xarray/plot/plot.py

Lines 672 to 673 in 648e13b

	if row or col:
	allargs = locals().copy()

If LooseVersion and mpl are not deleted, they are in the local environment, and end up in allargs, but are not valid keyword arguments. Moving the LooseVersion import to the top of the file fixes that, but I guess we don't want to do the same with mpl?

[mathause]

Why not? I would not expect an interaction here? Or is it because you don't actually call the code determining the cmap?

[johnomotani]

I don't understand the error. If I don't modify the test, I get

================================================================================================================== FAILURES ==================================================================================================================
__________________________________________________________________________________________________ TestSurface.test_seaborn_palette_as_cmap __________________________________________________________________________________________________

self = <xarray.tests.test_plot.TestSurface object at 0x7f5043826220>

    @requires_seaborn
    def test_seaborn_palette_as_cmap(self):
>       cmap_name = self.plotmethod(levels=2, cmap="husl").get_cmap().name

.../xarray/xarray/tests/test_plot.py:1232: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.../xarray/xarray/plot/plot.py:873: in plotmethod
    return newplotfunc(**allargs)
.../xarray/xarray/plot/plot.py:775: in newplotfunc
    primitive = plotfunc(
.../xarray/xarray/plot/plot.py:1035: in surface
    primitive = ax.plot_surface(x, y, z, **kwargs)
.../python3.8/site-packages/matplotlib/_api/deprecation.py:431: in wrapper
    return func(*inner_args, **inner_kwargs)
.../python3.8/site-packages/mpl_toolkits/mplot3d/axes3d.py:1742: in plot_surface
    polyc = art3d.Poly3DCollection(polys, *args, **kwargs)
.../python3.8/site-packages/mpl_toolkits/mplot3d/art3d.py:715: in __init__
    super().__init__(verts, *args, **kwargs)
.../python3.8/site-packages/matplotlib/collections.py:1206: in __init__
    super().__init__(**kwargs)
.../python3.8/site-packages/matplotlib/_api/deprecation.py:431: in wrapper
    return func(*inner_args, **inner_kwargs)
.../python3.8/site-packages/matplotlib/collections.py:164: in __init__
    cm.ScalarMappable.__init__(self, norm, cmap)
.../python3.8/site-packages/matplotlib/cm.py:263: in __init__
    self.set_cmap(cmap)  # The Colormap instance of this ScalarMappable.
.../python3.8/site-packages/matplotlib/cm.py:432: in set_cmap
    cmap = get_cmap(cmap)
.../python3.8/site-packages/matplotlib/cm.py:190: in get_cmap
    _api.check_in_list(sorted(_cmap_registry), name=name)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

_values = ['Accent', 'Accent_r', 'Blues', 'Blues_r', 'BrBG', 'BrBG_r', ...], _print_supported_values = True, kwargs = {'name': 'husl'}, values = ['Accent', 'Accent_r', 'Blues', 'Blues_r', 'BrBG', 'BrBG_r', ...], key = 'name', val = 'husl'

    def check_in_list(_values, *, _print_supported_values=True, **kwargs):
        """
        For each *key, value* pair in *kwargs*, check that *value* is in *_values*.
    
        Parameters
        ----------
        _values : iterable
            Sequence of values to check on.
        _print_supported_values : bool, default: True
            Whether to print *_values* when raising ValueError.
        **kwargs : dict
            *key, value* pairs as keyword arguments to find in *_values*.
    
        Raises
        ------
        ValueError
            If any *value* in *kwargs* is not found in *_values*.
    
        Examples
        --------
        >>> _api.check_in_list(["foo", "bar"], arg=arg, other_arg=other_arg)
        """
        values = _values
        for key, val in kwargs.items():
            if val not in values:
                if _print_supported_values:
>                   raise ValueError(
                        f"{val!r} is not a valid value for {key}; "
                        f"supported values are {', '.join(map(repr, values))}")
E                   ValueError: 'husl' is not a valid value for name; supported values are 'Accent', 'Accent_r', 'Blues', 'Blues_r', 'BrBG', 'BrBG_r', 'BuGn', 'BuGn_r', 'BuPu', 'BuPu_r', 'CMRmap', 'CMRmap_r', 'Dark2', 'Dark2_r', 'GnBu', 'GnBu_r', 'Greens', 'Greens_r', 'Greys', 'Greys_r', 'OrRd', 'OrRd_r', 'Oranges', 'Oranges_r', 'PRGn', 'PRGn_r', 'Paired', 'Paired_r', 'Pastel1', 'Pastel1_r', 'Pastel2', 'Pastel2_r', 'PiYG', 'PiYG_r', 'PuBu', 'PuBuGn', 'PuBuGn_r', 'PuBu_r', 'PuOr', 'PuOr_r', 'PuRd', 'PuRd_r', 'Purples', 'Purples_r', 'RdBu', 'RdBu_r', 'RdGy', 'RdGy_r', 'RdPu', 'RdPu_r', 'RdYlBu', 'RdYlBu_r', 'RdYlGn', 'RdYlGn_r', 'Reds', 'Reds_r', 'Set1', 'Set1_r', 'Set2', 'Set2_r', 'Set3', 'Set3_r', 'Spectral', 'Spectral_r', 'Wistia', 'Wistia_r', 'YlGn', 'YlGnBu', 'YlGnBu_r', 'YlGn_r', 'YlOrBr', 'YlOrBr_r', 'YlOrRd', 'YlOrRd_r', 'afmhot', 'afmhot_r', 'autumn', 'autumn_r', 'binary', 'binary_r', 'bone', 'bone_r', 'brg', 'brg_r', 'bwr', 'bwr_r', 'cividis', 'cividis_r', 'cool', 'cool_r', 'coolwarm', 'coolwarm_r', 'copper', 'copper_r', 'crest', 'crest_r', 'cubehelix', 'cubehelix_r', 'flag', 'flag_r', 'flare', 'flare_r', 'gist_earth', 'gist_earth_r', 'gist_gray', 'gist_gray_r', 'gist_heat', 'gist_heat_r', 'gist_ncar', 'gist_ncar_r', 'gist_rainbow', 'gist_rainbow_r', 'gist_stern', 'gist_stern_r', 'gist_yarg', 'gist_yarg_r', 'gnuplot', 'gnuplot2', 'gnuplot2_r', 'gnuplot_r', 'gray', 'gray_r', 'hot', 'hot_r', 'hsv', 'hsv_r', 'icefire', 'icefire_r', 'inferno', 'inferno_r', 'jet', 'jet_r', 'magma', 'magma_r', 'mako', 'mako_r', 'nipy_spectral', 'nipy_spectral_r', 'ocean', 'ocean_r', 'pink', 'pink_r', 'plasma', 'plasma_r', 'prism', 'prism_r', 'rainbow', 'rainbow_r', 'rocket', 'rocket_r', 'seismic', 'seismic_r', 'spring', 'spring_r', 'summer', 'summer_r', 'tab10', 'tab10_r', 'tab20', 'tab20_r', 'tab20b', 'tab20b_r', 'tab20c', 'tab20c_r', 'terrain', 'terrain_r', 'turbo', 'turbo_r', 'twilight', 'twilight_r', 'twilight_shifted', 'twilight_shifted_r', 'viridis', 'viridis_r', 'vlag', 'vlag_r', 'winter', 'winter_r'

.../python3.8/site-packages/matplotlib/_api/__init__.py:126: ValueError

[Illviljan]

I think you can remove all those matplotlib checks if you do something like this:

    subplot_kws = dict()
    if ax is None:
        # TODO: Importing Axes3D is not necessary in matplotlib >= 3.2.
        # Remove when minimum requirement of matplotlib is 3.2:
        from mpl_toolkits.mplot3d import Axes3D  # type: ignore # noqa

        subplot_kws.update(projection="3d")

I got stuck with the same thing in #4909. Importing Axes3D was the the standard way of turning on the 3d plotting functionalities. Now with newer releases it's not necessary anymore, but it's a backwards friendly way of doing it for a while longer.

[mathause]

Could you merge master so we don't get the changes from #5099. I can then have another look.

[johnomotani]

Thanks @mathause - master merged now.

[mathause]

Some more minor comments. It's good to go from my side afterwards. Maybe @dcherian wants to have a look?

[mathause]

Great! I can merge in a few days unless someone else has comments. One thing that could be nice is an example in http://xarray.pydata.org/en/stable/plotting.html - up to you.

[johnomotani]

One thing that could be nice is an example in http://xarray.pydata.org/en/stable/plotting.html - up to you.

It seemed odd to add a surface example when contour and countourf weren't shown - so I've added a new subsection for 'Other types of plot' and shown all three.

[mathause]

Once again, thanks a lot for your contribution @johnomotani! (the test failure is unrelated)