-
Notifications
You must be signed in to change notification settings - Fork 439
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
Move Matplotlib to hard requirement #4156
Conversation
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.
Agreed. It's also a vtk
requirement, so people already have to install this unless they ignore dependencies.
Codecov Report
@@ Coverage Diff @@
## main #4156 +/- ##
=======================================
Coverage 95.58% 95.59%
=======================================
Files 95 95
Lines 20356 20333 -23
=======================================
- Hits 19458 19438 -20
+ Misses 898 895 -3 |
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.
If we make matplotlib a hard dependency then we should at the same time tear out a bunch of logic related to the soft dependency status:
$ git grep matplotlib 'pyvista/*.py'
pyvista/core/filters/data_set.py: Plot the variables of interest in 2D using matplotlib where the
pyvista/core/filters/data_set.py: The string title of the matplotlib figure.
pyvista/core/filters/data_set.py: Shows the matplotlib figure.
pyvista/core/filters/data_set.py: # Ensure matplotlib is available
pyvista/core/filters/data_set.py: import matplotlib.pyplot as plt
pyvista/core/filters/data_set.py: raise ImportError('matplotlib must be available to use this filter.')
pyvista/core/filters/data_set.py: The string title of the ``matplotlib`` figure.
pyvista/core/filters/data_set.py: Shows the ``matplotlib`` figure when ``True``.
pyvista/core/filters/data_set.py: # Ensure matplotlib is available
pyvista/core/filters/data_set.py: import matplotlib.pyplot as plt
pyvista/core/filters/data_set.py: raise ImportError('matplotlib must be installed to use this filter.')
pyvista/core/filters/data_set.py: # create the matplotlib figure
pyvista/core/filters/data_set.py: The string title of the `matplotlib` figure.
pyvista/core/filters/data_set.py: Shows the matplotlib figure.
pyvista/core/filters/data_set.py: # Ensure matplotlib is available
pyvista/core/filters/data_set.py: import matplotlib.pyplot as plt
pyvista/core/filters/data_set.py: raise ImportError('matplotlib must be installed to use this filter.')
pyvista/core/filters/data_set.py: # create the matplotlib figure
pyvista/demos/demos.py: import matplotlib # noqa
pyvista/jupyter/pv_ipygany.py: # attempt to matplotlib cmaps to ipygany
pyvista/plotting/charts.py: """2D chart class similar to a ``matplotlib`` figure.
pyvista/plotting/charts.py: the `matplotlib.pyplot.plot
pyvista/plotting/charts.py: <https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.plot.html>`_.
pyvista/plotting/charts.py: """Create new chart from an existing matplotlib figure.
pyvista/plotting/charts.py: figure : matplotlib.figure.Figure, optional
pyvista/plotting/charts.py: The matplotlib figure to draw. If no figure is
pyvista/plotting/charts.py: Plot streamlines of a vector field with varying colors (based on `this example <https://matplotlib.org/stable/gallery/images_contours_and_fields/plot_streamplot.html>`_).
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/charts.py: "chart_name": "matplotlib chart",
pyvista/plotting/charts.py: from matplotlib.backends.backend_agg import FigureCanvasAgg
pyvista/plotting/charts.py: import matplotlib.figure # noqa
pyvista/plotting/charts.py: import matplotlib.pyplot as plt
pyvista/plotting/charts.py: raise ImportError("ChartMPL requires matplotlib")
pyvista/plotting/charts.py: # Close the underlying matplotlib figure when creating the sphinx gallery.
pyvista/plotting/charts.py: # matplotlib figure (fetched by the 'matplotlib' scraper).
pyvista/plotting/charts.py: """Retrieve the matplotlib figure associated with this chart.
pyvista/plotting/charts.py: Create a matplotlib chart from an existing figure.
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/charts.py: Plotter window is resized or the matplotlib figure is
pyvista/plotting/charts.py: Create a matplotlib chart with title 'My Chart'.
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/charts.py: Create a matplotlib chart with custom labels and show the legend.
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/colors.py:Used code from matplotlib.colors. Thanks for your work.
pyvista/plotting/colors.py:# Following colors are copied from matplotlib.colors, synonyms (colors with a
pyvista/plotting/colors.py:matplotlib_default_colors = [
pyvista/plotting/colors.py: """Fetch a colormap by name from matplotlib, colorcet, or cmocean."""
pyvista/plotting/colors.py: if not has_module('matplotlib'):
pyvista/plotting/colors.py: 'The use of custom colormaps requires the installation of matplotlib.'
pyvista/plotting/colors.py: from matplotlib import colormaps, colors
pyvista/plotting/colors.py: import matplotlib
pyvista/core/filters/data_set.py: Plot the variables of interest in 2D using matplotlib where the
pyvista/core/filters/data_set.py: The string title of the matplotlib figure.
pyvista/core/filters/data_set.py: Shows the matplotlib figure.
pyvista/core/filters/data_set.py: # Ensure matplotlib is available
pyvista/core/filters/data_set.py: import matplotlib.pyplot as plt
pyvista/core/filters/data_set.py: raise ImportError('matplotlib must be available to use this filter.')
pyvista/core/filters/data_set.py: The string title of the ``matplotlib`` figure.
pyvista/core/filters/data_set.py: Shows the ``matplotlib`` figure when ``True``.
pyvista/core/filters/data_set.py: # Ensure matplotlib is available
pyvista/core/filters/data_set.py: import matplotlib.pyplot as plt
pyvista/core/filters/data_set.py: raise ImportError('matplotlib must be installed to use this filter.')
pyvista/core/filters/data_set.py: # create the matplotlib figure
pyvista/core/filters/data_set.py: The string title of the `matplotlib` figure.
pyvista/core/filters/data_set.py: Shows the matplotlib figure.
pyvista/core/filters/data_set.py: # Ensure matplotlib is available
pyvista/core/filters/data_set.py: import matplotlib.pyplot as plt
pyvista/core/filters/data_set.py: raise ImportError('matplotlib must be installed to use this filter.')
pyvista/core/filters/data_set.py: # create the matplotlib figure
pyvista/demos/demos.py: import matplotlib # noqa
pyvista/jupyter/pv_ipygany.py: # attempt to matplotlib cmaps to ipygany
pyvista/plotting/charts.py: """2D chart class similar to a ``matplotlib`` figure.
pyvista/plotting/charts.py: the `matplotlib.pyplot.plot
pyvista/plotting/charts.py: <https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.plot.html>`_.
pyvista/plotting/charts.py: """Create new chart from an existing matplotlib figure.
pyvista/plotting/charts.py: figure : matplotlib.figure.Figure, optional
pyvista/plotting/charts.py: The matplotlib figure to draw. If no figure is
pyvista/plotting/charts.py: Plot streamlines of a vector field with varying colors (based on `this example <https://matplotlib.org/stable/gallery/images_contours_and_fields/plot_streamplot.html>`_).
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/charts.py: "chart_name": "matplotlib chart",
pyvista/plotting/charts.py: from matplotlib.backends.backend_agg import FigureCanvasAgg
pyvista/plotting/charts.py: import matplotlib.figure # noqa
pyvista/plotting/charts.py: import matplotlib.pyplot as plt
pyvista/plotting/charts.py: raise ImportError("ChartMPL requires matplotlib")
pyvista/plotting/charts.py: # Close the underlying matplotlib figure when creating the sphinx gallery.
pyvista/plotting/charts.py: # matplotlib figure (fetched by the 'matplotlib' scraper).
pyvista/plotting/charts.py: """Retrieve the matplotlib figure associated with this chart.
pyvista/plotting/charts.py: Create a matplotlib chart from an existing figure.
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/charts.py: Plotter window is resized or the matplotlib figure is
pyvista/plotting/charts.py: Create a matplotlib chart with title 'My Chart'.
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/charts.py: Create a matplotlib chart with custom labels and show the legend.
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/colors.py:Used code from matplotlib.colors. Thanks for your work.
pyvista/plotting/colors.py:# Following colors are copied from matplotlib.colors, synonyms (colors with a
pyvista/plotting/colors.py:matplotlib_default_colors = [
pyvista/plotting/colors.py: """Fetch a colormap by name from matplotlib, colorcet, or cmocean."""
pyvista/plotting/colors.py: if not has_module('matplotlib'):
pyvista/plotting/colors.py: 'The use of custom colormaps requires the installation of matplotlib.'
pyvista/plotting/colors.py: from matplotlib import colormaps, colors
pyvista/plotting/colors.py: import matplotlib
pyvista/plotting/colors.py: if not scooby.meets_version(matplotlib.__version__, min_req):
pyvista/plotting/colors.py: f'matplotlib>={min_req}.'
pyvista/plotting/colors.py: if not has_module('matplotlib'):
pyvista/plotting/colors.py: 'The use of custom colormaps requires the installation of matplotlib.'
pyvista/plotting/colors.py: from matplotlib.colors import ListedColormap
pyvista/plotting/colors.py: """Return the default color cycler (matches matplotlib's default)."""
pyvista/plotting/colors.py: raise ImportError('cycler not installed. Please install matplotlib.')
pyvista/plotting/colors.py: return cycler('color', matplotlib_default_colors)
pyvista/plotting/colors.py: raise ImportError('cycler not installed. Please install matplotlib.')
pyvista/plotting/colors.py:def get_matplotlib_theme_cycler():
pyvista/plotting/colors.py: """Return matplotlib's current theme's color cycler."""
pyvista/plotting/colors.py: import matplotlib.pyplot as plt
pyvista/plotting/colors.py: raise ImportError('Please install matplotlib.')
pyvista/plotting/colors.py: raise ImportError('cycler not installed. Please install matplotlib.')
pyvista/plotting/colors.py: * ``'default'`` - Use the default color cycler (matches matplotlib's default)
pyvista/plotting/colors.py: * ``'matplotlib`` - Dynamically get matplotlib's current theme's color cycler.
pyvista/plotting/colors.py: raise ImportError('cycler not installed. Please install matplotlib.')
pyvista/plotting/colors.py: elif color_cycler == 'matplotlib':
pyvista/plotting/colors.py: return get_matplotlib_theme_cycler()
pyvista/plotting/composite_mapper.py: This uses ``matplotlib``'s color cycler.
pyvista/plotting/composite_mapper.py: if has_module('matplotlib'):
pyvista/plotting/composite_mapper.py: import matplotlib
pyvista/plotting/composite_mapper.py: colors = cycle(matplotlib.rcParams['axes.prop_cycle'])
pyvista/plotting/composite_mapper.py: warnings.warn('Please install matplotlib for color cycles.')
pyvista/plotting/composite_mapper.py: if cmap is None: # Set default map if matplotlib is available
pyvista/plotting/composite_mapper.py: if has_module('matplotlib'):
pyvista/plotting/lookup_table.py: Color map from ``matplotlib``, ``colorcet``, or ``cmocean``. Either
pyvista/plotting/lookup_table.py: Apply ``matplotlib``'s ``'cividis'`` color map.
pyvista/plotting/lookup_table.py: if not has_module('matplotlib'): # pragma: no cover
pyvista/plotting/mapper.py: # Set default map if matplotlib is available
pyvista/plotting/mapper.py: if has_module('matplotlib'):
pyvista/plotting/mapper.py: # no requires matplotlib
pyvista/plotting/plotting.py: If a string, this is the name of the ``matplotlib`` colormap to use
pyvista/plotting/plotting.py: Color each block by a solid color using matplotlib's color cycler.
pyvista/plotting/plotting.py: If a string, this is the name of the ``matplotlib`` colormap to use
pyvista/plotting/plotting.py: each block by a solid color using matplotlib's color cycler.
pyvista/plotting/plotting.py: If a string, this is the name of the ``matplotlib`` colormap to use
pyvista/plotting/plotting.py: if not has_module('matplotlib'):
pyvista/plotting/plotting.py: raise ImportError('Please install matplotlib for color maps.')
pyvista/plotting/renderer.py: * ``'default'`` - Use the default color cycler (matches matplotlib's default)
pyvista/plotting/renderer.py: * ``'matplotlib`` - Dynamically get matplotlib's current theme's color cycler.
pyvista/plotting/renderer.py: ``matplotlib``'s ``grid`` function.
pyvista/plotting/renderer.py: list, or a matplotlib color string (e.g. ``'w'`` or ``'white'``
pyvista/plotting/renderers.py: * ``'default'`` - Use the default color cycler (matches matplotlib's default)
pyvista/plotting/renderers.py: * ``'matplotlib`` - Dynamically get matplotlib's current theme's color cycler.
pyvista/themes.py: 'Unable to set a default theme colormap without matplotlib. '
pyvista/themes.py: * ``'default'`` - Use the default color cycler (matches matplotlib's default)
pyvista/themes.py: * ``'matplotlib`` - Dynamically get matplotlib's current theme's color cycler.
pyvista/utilities/errors.py: matplotlib : 3.6.0
pyvista/utilities/errors.py: 'matplotlib',
pyvista/utilities/misc.py: """Return if a figure can be created with matplotlib."""
pyvista/utilities/misc.py: import matplotlib.pyplot as plt
pyvista/utilities/sphinx_gallery.py: 'matplotlib' and 'mayavi'. Details on this implementation can be found in
A bunch of the above lines signal snippets where we go out of our way to check if matplotlib is installed. We should remove those rather then let them sit around as debt.
Here's a filter for the imports alone, but this misses some cases, e.g. cycler
availability checking:
$ git grep 'from matplotlib\|import matplotlib' 'pyvista/*.py'
pyvista/core/filters/data_set.py: import matplotlib.pyplot as plt
pyvista/core/filters/data_set.py: import matplotlib.pyplot as plt
pyvista/core/filters/data_set.py: import matplotlib.pyplot as plt
pyvista/demos/demos.py: import matplotlib # noqa
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/charts.py: from matplotlib.backends.backend_agg import FigureCanvasAgg
pyvista/plotting/charts.py: import matplotlib.figure # noqa
pyvista/plotting/charts.py: import matplotlib.pyplot as plt
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/charts.py: >>> import matplotlib.pyplot as plt
pyvista/plotting/colors.py:Used code from matplotlib.colors. Thanks for your work.
pyvista/plotting/colors.py:# Following colors are copied from matplotlib.colors, synonyms (colors with a
pyvista/plotting/colors.py: """Fetch a colormap by name from matplotlib, colorcet, or cmocean."""
pyvista/plotting/colors.py: from matplotlib import colormaps, colors
pyvista/plotting/colors.py: import matplotlib
pyvista/plotting/colors.py: from matplotlib.colors import ListedColormap
pyvista/plotting/colors.py: import matplotlib.pyplot as plt
pyvista/plotting/composite_mapper.py: import matplotlib
pyvista/utilities/misc.py: import matplotlib.pyplot as plt
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.
Great job tearing out the legacy @tkoyama010 :) I have a few comments, and I think there's a few more things we could/should get rid of:
$ git grep -i matplotlib |grep -in 'require\|installed'
28:docker/requirements.txt:matplotlib
54:examples/02-plot/color_cycler.py:# This requires matplotlib (or at least cycler) to be installed.
71:examples_trame/requirements.txt:matplotlib
133:pyvista/plotting/composite_mapper.py: for when displaying ``scalars``. Requires Matplotlib to be
142:pyvista/plotting/mapper.py: for when displaying ``scalars``. Requires Matplotlib to be
145:pyvista/plotting/plotting.py: Matplotlib to be installed. ``colormap`` is also an accepted alias
149:pyvista/plotting/plotting.py: Matplotlib to be installed. ``colormap`` is also an accepted alias
153:pyvista/plotting/plotting.py: Matplotlib to be installed. ``colormap`` is also an accepted alias
161:pyvista/themes.py: displaying ``scalars``. Requires Matplotlib to be installed.
167:requirements.txt:matplotlib>=3.5.0,<3.7.2
179:tests/plotting/test_plotting.py: reason='VTK and Matplotlib version incompatibility. For VTK<=9.2.2, MathText requires matplotlib<3.6',
180:tests/plotting/test_plotting.py: For VTK<=9.2.2, this requires matplotlib<3.6
193:tests/test_filters.py: """This test requires matplotlib."""
194:tests/test_filters.py: """This test requires matplotlib."""
195:tests/test_filters.py: """This test requires matplotlib."""
Most of these signals stale comments or remarks in docstrings, and stale requirements because matplotlib is now a direct dependency of pyvista.
@@ -713,7 +712,6 @@ def set_scalars( | |||
self.lookup_table.apply_cmap(cmap, n_colors) | |||
else: # pragma: no cover | |||
# should be impossible to get to this point as VTK package | |||
# no requires matplotlib |
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.
I think this was a typo, and the second part of this sentence was meant to be
# now requires matplotlib
In which case we should restore this line and fix the typo. Or reword the two-line comment to mention our own matplotlib dependency. Something like
# should be impossible to get to this point as matplotlib is a dependency
(give or take line lengths and wording).
Or perhaps the whole comment is meaningless now, I haven't checked the big picture. In this case I'd raise in the else
, maybe an AssertionError
to signal that this should never happen.
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.
Turns out you can trigger this section of code even with matplotlib
installed with:
import pyvista
pyvista.global_theme.cmap = None
mesh = pyvista.Sphere()
mesh.plot(scalars=range(mesh.n_points))
This is because by setting pyvista.global_theme.cmap = None
, now it's possible for cmap
to be None
within the mapper.
The easiest fix is to make sure that our theme must have a valid default color map. Since we now require matplotlib
, we can do this and completely remove this section of code.
@tkoyama010, I'm going to add support for |
I'd prefer not to bound the version of matplotlib if possible. I think there are really only two things where the version matters:
Instead, can we support the color mapping API across versions and have a helper utility to warn about incompatible VTK and MPL versions with MathText |
Fully agree.
Fully agree. I'll deal with (1), can you deal with (2)? |
Will do -- let me know when you're wrapped up, and I will improve the MathText support utils with:
|
Co-authored-by: Andras Deak <adeak@users.noreply.github.com>
Phase 1 with older matplotlib compatibility looks good to me, thanks @akaszynski. There's one test failure with matplotlib 3.4:
This is because we get the wrong kind of error here: Lines 311 to 312 in bffe561
So this is a testing issue rather than a library issue, and we don't want to make the test suite work for older matplotlib (I assume). On second thought, this might actually be an issue. Setting a non-existent cmap on a theme should probably be more informative: >>> import pyvista as pv
>>> pv.global_theme.cmap = 'potato'
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/adeak/pyvista/pyvista/themes.py", line 2039, in cmap
get_cmap_safe(cmap) # for validation
File "/home/adeak/pyvista/pyvista/plotting/colors.py", line 1221, in get_cmap_safe
cmap = colormaps[cmap]
TypeError: 'module' object is not subscriptable |
Sorry, I didn't realize the discussion was still going on and deleted |
Co-authored-by: Andras Deak <adeak@users.noreply.github.com>
Resolved the I also added a lower limit in
Considering that |
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.
Great work all. Nice to see net decreases of code without removing functionality.
There was one unresolved issue that I've opened issue #4174 for, but otherwise this is good to go.
Thank you all for reviews, comments, and support. I could not have done it without your help. |
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.
LGTM, thanks a lot @tkoyama010!
Overview
resolves #3970
Details