Mnt deprecate mlab #22920

jklymak · 2022-04-27T19:27:46Z

PR Summary

These are all somewhat fragile wrappers around functionality that rightly belongs elsewhere in the python scientific stack. There is nothing particular to plotting in these routines, and simply predate widely available scipy.signal and scipy.stats. I guess some one could make a new toolbox, but I'm not sure what would be in it...

Obviously this needs wide agreement before proceeding.

Draft until I can see what fails.

PR Checklist

Tests and Styling

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs should build without error).

tacaswell · 2022-04-27T19:54:56Z

lib/matplotlib/mlab.py

@@ -840,6 +846,7 @@ def cohere(x, y, NFFT=256, Fs=2, detrend=detrend_none, window=window_hanning,
    return Cxy, f


+@_api.deprecated("3.6", alternative="scipy.stats.gaussian_kde")


We use this internally for violin plots iirc...

yeah, I was just abusing CI and waiting for things to fail before figuring what needed to be changed elsewhere. Worst-case if I can't make KDEs work directly from scipy I'd still suggest we deprecate this as a public method.

oh, hold it, do we still not require scipy as a hard requirement? I guess that means violinplots need us to vendor this?

OK backed out this change. I didn't make this private either because the docs leave it possible to pass a GaussianKDE object to violinplots.

oscargus · 2022-04-27T21:34:13Z

lib/matplotlib/pyplot.py

@@ -2900,16 +2895,14 @@ def streamplot(
        x, y, u, v, density=1, linewidth=None, color=None, cmap=None,
        norm=None, arrowsize=1, arrowstyle='-|>', minlength=0.1,
        transform=None, zorder=None, start_points=None, maxlength=4.0,
-        integration_direction='both', broken_streamlines=True, *,


This change seems strange.

ooops, Looks like I deleted it by accident. Thanks for catching....

Oh, hmmm. I'm not doing this - its boilerplate.py. I'm not actually clear what is going on here....

Strange. Is boilerplate executed automatically or manually? (Sorry for my ignorance.)

Manually. This commit I've not executed boilerplate, and it seems better plus or minus the font-finding problem, which I believe is unrelated

jklymak · 2022-04-28T20:26:08Z

Discussed on call. OK to go ahead with this. Vendor GaussianKDE and make the current version private. @oscargus if you were still interested in pulling this out as a toolbox that would be fine as well, but we will deprecate here. Maybe if the toolbox is set up we can change the deprecation messages to point to it?

oscargus · 2022-04-28T21:36:51Z

@jklymak I'm interested in a toolbox with related functions, yes. While these may not be the ones I use and focus, it may very much make sense to add them there.

I'll see if I can come up with something quite soon, at least before 3.6... Not sure if we should wait with merging this until we can point to it (matplotlib/mpl-signal or something) or if we simply should prepare a new PR later on and merge this when (if) decided.

jklymak · 2022-04-28T21:56:22Z

I don't think there is a rush for the deprecation if you think you will have something in a couple of months.

anntzer · 2022-04-28T22:13:34Z

oh, hold it, do we still not require scipy as a hard requirement

I'm certainly OK with deprecating spectral functions from mlab, but I would much rather not add a dependency on scipy. While conda is mostly always available these days, in the rare cases where it's not and no wheels are available (e.g. shortly after new python releases), compiling scipy can be a huge pain (due to the fortran parts), much worse than numpy and matplotlib.

jklymak · 2022-04-28T22:18:12Z

Yes, that was the consensus on the call - just vendor GaussianKDE. The other functions that should depend on scipy, but do not, will just be flat out deprecated, and maybe moved to a different toolbox if people really complain about the lack of a psd function.

tacaswell · 2022-04-28T22:37:14Z

If we want to have a signal related toolkit it makes the most sense for it to be a stand-alone package so you can apologetically depend on scipy!

oscargus · 2022-05-07T09:01:39Z

Started a bit on the toolbox: https://github.com/oscargus/mplsignal so can easily move relevant functions there when needed. (I expect to maybe release a first version in a few weeks and may ask to have it move to the mpl organization before that as I expect to move it at some stage anyway,)

Btw, the idea is to not depend on scipy there either (even though some functions are customized to fit exactly into some scipy-functions like freqz that has a plot-callback), so at least provide some fallback functions.

jklymak · 2022-05-10T15:57:19Z

I'm completely befuddled as to why this PR fails font-name tests. I rebased off master, and I don't see any way I touched font handling. Something weird is going on...

oscargus · 2022-06-05T11:34:11Z

I realized that we either should deprecate the corresponding functions in pyplot/Axes as well or make sure that they use the non-deprecated versions directly from numpy (where possible) or scipy (and fail gracefully if it is not installed).

I could open a PR using the numpy functions directly if that is the preferred solution (but will not spend time otherwise).

Edit: I was a bit too quick here and realize that it is only xcorr and cohere that relies on mlab. Still it would require some change. Probably we should keep windows and detrends?

Edit 2: But considering that the tests are removed, which are really tests for pyplot functions, there is still an issue to consider here.

jklymak · 2022-06-07T11:06:02Z

I realized that we either should deprecate the corresponding functions in pyplot/Axes as well or make sure that they use the non-deprecated versions directly from numpy (where possible) or scipy (and fail gracefully if it is not installed).

We should definitely deprecate those!

...done.

anntzer · 2022-07-19T15:43:30Z

What's the status here? Is this dependent on mplsignal being released on PyPI? (It doesn't seem to be there yet.)
Normally the tests should only be removed when the implementations are removed too (they should be protected with with pytest.warns(...): in the meantime)?

anntzer · 2022-07-19T15:44:06Z

lib/matplotlib/mlab.py

-commands with the same names. Most numerical Python functions can be found in
-the `NumPy`_ and `SciPy`_ libraries. What remains here is code for performing
-spectral computations and kernel density estimations.
+This module is deprecated in favour of modules that can be found in


In the long-term, will GaussianKDE live in another module? Or will mlab stay around to host it?

jklymak · 2022-07-20T01:28:04Z

What's the status here? Is this dependent on mplsignal being released on PyPI? (It doesn't seem to be there yet.)

I don't think this should wait for a replacement package.

Normally the tests should only be removed when the implementations are removed too (they should be protected with with pytest.warns(...): in the meantime)?

OK, I can do that

In the long-term, will GaussianKDE live in another module? Or will mlab stay around to host it?

It probably should get put somewhere else, but seems a bit cumbersome to move it now.

jklymak · 2022-07-20T01:39:36Z

Normally the tests should only be removed when the implementations are removed too (they should be protected with with pytest.warns(...): in the meantime)?

OK, I can do that

Actually thats a pretty huge PITA - test_mlab has >100 tests, most of which now fail due to the deprecation warning.... Surely I don't have to wrap them all in a with pytest.warns(...): ?

anntzer · 2022-07-20T07:05:24Z

I think(?) you can use a pytestmark (https://docs.pytest.org/en/7.1.x/example/markers.html#marking-whole-classes-or-modules) with a custom warningsfilter mark (https://docs.pytest.org/en/7.1.x/how-to/capture-warnings.html#pytest-mark-filterwarnings, see also https://docs.python.org/3/library/warnings.html#describing-warning-filters for syntax).

jklymak · 2022-07-20T21:00:41Z

OK, tests marked to filter deprecation warnings. I assume we don't need to wrap the individual calls to allow other deprecation warnings to be caught, so this should be fine. This needs a rebase before merging to undo the files that were removed and then added.

anntzer · 2022-07-20T21:07:33Z

pytest.ini

@@ -1,12 +0,0 @@
-# Because tests can be run from an installed copy, most of our Pytest


This file should stay.

anntzer · 2022-07-20T21:08:55Z

lib/matplotlib/tests/test_mlab.py

@@ -1,1074 +0,0 @@
-from numpy.testing import (assert_allclose, assert_almost_equal,


My suggestion was to keep this as well (with a global pytestmark to filter warnings everywhere in the file), but I guess the Axes tests essentially cover what we want too?

Yep, sorry, I forgot I had to "add" it...

.... except for GaussianKDE, which is needed for violinplots

QuLogic · 2022-07-21T00:01:51Z

lib/matplotlib/mlab.py

@@ -69,6 +68,7 @@ def window_hanning(x):
    return np.hanning(len(x))*x


+@_api.deprecated("3.6", alternative="")


Missing alternative.

Its a no-op? I guess I could say lambda x: x.

alternative="" is the default. You don't have to specify this explicitly.

I'm fine either way: Don't give an alternative (we don't have to specify one). Or use lambda x: x.

QuLogic · 2022-07-21T00:03:03Z

lib/matplotlib/mlab.py

@@ -157,6 +159,7 @@ def detrend_mean(x, axis=None):
    return x - x.mean(axis, keepdims=True)


+@_api.deprecated("3.6", alternative="scipy.signal.detrend")


That doesn't seem to be right alternative?

jklymak · 2022-11-11T20:01:25Z

This one is a bit annoying to drag from milestone to milestone. I still feel we should deprecate all this, but if we aren't going to do it, then let's close the PR. Thanks!

jklymak · 2022-12-11T21:52:34Z

... we are about to feature freeze for 3.7 - if we would like this change, now is the time to do it, otherwise, we can close this...

oscargus · 2022-12-12T08:34:31Z

I should be able to get it into mplsignal and make a first release during the holidays.

jklymak · 2022-12-15T17:29:20Z

OK, I'll close this as I don't see it getting in before 3.7 is frozen, and I'm not willing to rebase it to 3.8. I think we should not be providing this, but understand if folks want to keep it around.

jklymak added the status: needs comment/discussion needs consensus on next step label Apr 27, 2022

tacaswell reviewed Apr 27, 2022

View reviewed changes

oscargus reviewed Apr 27, 2022

View reviewed changes

jklymak force-pushed the mnt-deprecate-mlab branch from 29710aa to 3d0b2e3 Compare April 27, 2022 23:00

jklymak force-pushed the mnt-deprecate-mlab branch from 604b2b0 to ca393ec Compare May 9, 2022 23:49

jklymak marked this pull request as ready for review May 9, 2022 23:49

jklymak added the status: needs review label May 9, 2022

jklymak force-pushed the mnt-deprecate-mlab branch from ca393ec to beaa1d0 Compare May 9, 2022 23:54

jklymak removed the status: needs comment/discussion needs consensus on next step label May 10, 2022

jklymak force-pushed the mnt-deprecate-mlab branch 2 times, most recently from d804f02 to 78aff43 Compare May 16, 2022 08:35

oscargus mentioned this pull request May 17, 2022

Slightly simplify tcl/tk load in extension. #23054

Merged

6 tasks

jklymak added this to the v3.6.0 milestone May 18, 2022

oscargus mentioned this pull request Jun 5, 2022

[Doc]: More plot types? #23204

Open

jklymak marked this pull request as draft June 7, 2022 11:06

jklymak removed the status: needs review label Jun 7, 2022

jklymak force-pushed the mnt-deprecate-mlab branch from 7b9aad9 to 21a6b93 Compare June 7, 2022 12:15

jklymak marked this pull request as ready for review June 7, 2022 12:51

jklymak force-pushed the mnt-deprecate-mlab branch from 21a6b93 to 06cd7c1 Compare June 8, 2022 09:22

jklymak force-pushed the mnt-deprecate-mlab branch from 06cd7c1 to 9aea3ed Compare June 30, 2022 09:13

jklymak mentioned this pull request Jul 17, 2022

Cleanup psd example. #23426

Closed

6 tasks

anntzer reviewed Jul 19, 2022

View reviewed changes

jklymak force-pushed the mnt-deprecate-mlab branch 2 times, most recently from 019b0e7 to 4e341cb Compare July 20, 2022 19:56

anntzer reviewed Jul 20, 2022

View reviewed changes

jklymak force-pushed the mnt-deprecate-mlab branch from 4e341cb to 5cf2f93 Compare July 20, 2022 21:11

MNT: deprecate mlab

41f946e

.... except for GaussianKDE, which is needed for violinplots

jklymak force-pushed the mnt-deprecate-mlab branch from 5cf2f93 to 41f946e Compare July 20, 2022 21:15

QuLogic reviewed Jul 21, 2022

View reviewed changes

jklymak force-pushed the mnt-deprecate-mlab branch 2 times, most recently from df4fe34 to cb27276 Compare July 22, 2022 00:34

FIX: detrends

59532ae

jklymak force-pushed the mnt-deprecate-mlab branch from cb27276 to 59532ae Compare July 22, 2022 15:45

QuLogic modified the milestones: v3.6.0, v3.7.0 Aug 24, 2022

Fix for 3.7

c6b33a6

jklymak closed this Dec 15, 2022

jklymak mentioned this pull request Dec 27, 2022

[Bug]: Possible problem with parameter NFFT and pad_to of mlab._spectral_helper #24822

Open

		@@ -840,6 +846,7 @@ def cohere(x, y, NFFT=256, Fs=2, detrend=detrend_none, window=window_hanning,
		return Cxy, f


		@_api.deprecated("3.6", alternative="scipy.stats.gaussian_kde")

		@@ -1,12 +0,0 @@
		# Because tests can be run from an installed copy, most of our Pytest

		@@ -1,1074 +0,0 @@
		from numpy.testing import (assert_allclose, assert_almost_equal,

		@@ -69,6 +68,7 @@ def window_hanning(x):
		return np.hanning(len(x))*x


		@_api.deprecated("3.6", alternative="")

		@@ -157,6 +159,7 @@ def detrend_mean(x, axis=None):
		return x - x.mean(axis, keepdims=True)


		@_api.deprecated("3.6", alternative="scipy.signal.detrend")

Mnt deprecate mlab #22920

Mnt deprecate mlab #22920

Conversation

jklymak commented Apr 27, 2022

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Apr 28, 2022

oscargus commented Apr 28, 2022

jklymak commented Apr 28, 2022

anntzer commented Apr 28, 2022 • edited Loading

jklymak commented Apr 28, 2022

tacaswell commented Apr 28, 2022

oscargus commented May 7, 2022

jklymak commented May 10, 2022

oscargus commented Jun 5, 2022 • edited Loading

jklymak commented Jun 7, 2022 • edited Loading

anntzer commented Jul 19, 2022

Choose a reason for hiding this comment

jklymak commented Jul 20, 2022

jklymak commented Jul 20, 2022

anntzer commented Jul 20, 2022

jklymak commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Nov 11, 2022

jklymak commented Dec 11, 2022

oscargus commented Dec 12, 2022

jklymak commented Dec 15, 2022

anntzer commented Apr 28, 2022 •

edited

Loading

oscargus commented Jun 5, 2022 •

edited

Loading

jklymak commented Jun 7, 2022 •

edited

Loading