Skip to content
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

Deprecate mpl.py (was Remove mpl.py) #1535

Closed
wants to merge 4 commits into from

Conversation

ivanov
Copy link
Member

@ivanov ivanov commented Nov 26, 2012

matplotlib/mpl.py is a file which gets imported via ipython's %pyplot interface as module named mpl, yet it isn't clear at all what it's role is (it has no docstring).

Given that in our own code, and in the rest of our python neighborhood, we often do
import matplotlib as mpl, it's quite a bit confusing to have something with the mpl name within matplotlib not be the matplotlib module itself.

This module was only used in the pylab interface, where it was simply imported in one line via from matplotlib import mpl and never used again. This PR changes that one line to be import matplotlib as mpl and removes matplotlib/mpl.py altogether

The only thing mpl.py that doesn't get imported if, in pylab.py, we change from matplotlib import mpl toand get rid of mpl.py altogether, is the from matplotlib import finance. I have added a commit here to retain that import, just in case. Howver, I'm not sure it's the right thing to do, because now it preserve a bit of a dichotomy between import matplotlib as mpl and from pylab import mpl, where the latter will have mpl.finance, and the former will not.

I'm open to removing 4863826 from this PR in order to not squelch this dichotomy, and document such potential backwards incompatibility instead.

UPDATED: switched to deprecation of mpl.py

matplotlib/mpl.py is a file which used to get imported via ipython's
%pyplot interface as module named mpl, yet it isn't clear at all
what it's role is.

Given that in our own code, and in the rest of our python neighborhood,
we often do import matplotlib as mpl, it's quite a bit confusing to
have something with the mpl name within matplotlib not be the
matplotlib module itself.

This module was only used in pylab interface, where it was simply
imported in one line via from matplotlib import mpl and never used
again. With the exception of mpl.finance, all of the modules in mpl.py
were readily available simply via "import matplotlib as mpl".

pylab's mpl.finance functionality is retained for backwards
compatability.

@dmcdougall
Copy link
Member

I'm fine with this. However, I didn't even know mpl.finance existed!

Does anybody else have any feedback?

@mdboom
Copy link
Member

mdboom commented Nov 26, 2012

Yeah -- I wasn't aware of this either. I think finance should probably be put in the pyplot namespace (which then gets pulled into pylab) rather than putting in in pylab though. But all this is based on absolutely zero knowledge of how this is being used outside of matplotlib.

@efiring
Copy link
Member

efiring commented Nov 26, 2012

I thought maybe I was the one who introduced mpl.py, but it looks like it was John, back in 2007. I don't recall the rationale. I use it in quite a few places in my own code. I don't mind changing my own code to accomodate its removal--I can do it easily, and will do so in any case--but I do wonder whether some other misguided soul might be using it, and might be puzzled by its disappearance with no deprecation process. After all, it is a whole module, even though it is undocumented and trivial in its content. (It might have been introduced so that one could import core matplotlib modules without importing the legacy numerix module. I haven't tried to diagnose whether that is actually its effect. It is part of a changeset that was phasing out numerix. I was quite involved in all the comings and goings with numerix, Numeric, numarray, numpy, and the variations of masked arrays.)

@dmcdougall
Copy link
Member

@efiring Maybe add a warning just to be careful?

@efiring
Copy link
Member

efiring commented Dec 3, 2012

Yes, I think the way to remove mpl.py is by starting with a deprecation warning.

@dmcdougall
Copy link
Member

I agree. That will save us a lot of mailing list traffic if a lot of people are using it.

@ivanov Shall I close this PR in favour of a deprecation, or would you just like to update and rebase this one?

@ivanov
Copy link
Member Author

ivanov commented Dec 3, 2012

I'll update and rebase in a couple of hours, that does sound like a more
prudent approach

`matplotlib/mpl.py` is a file which used to get imported via ipython's
`%pyplot` interface as module named `mpl`, yet it isn't clear at all
what it's role is.

Given that in our own code, and in the rest of our python neighborhood,
we often do `import matplotlib as mpl`, it's quite a bit confusing to
have something with the `mpl` name within matplotlib not be the
`matplotlib` module itself.

This module was only used in `pylab` interface, where it was simply
imported in one line via `from matplotlib import mpl` and never used
again. With the exception of mpl.finance, all of the modules in mpl.py
were readily available simply via "import matplotlib as mpl".

pylab's mpl.finance functionality is retained for backwards
compatability.
@ivanov
Copy link
Member Author

ivanov commented Dec 5, 2012

ok, I've added the deprecation warning. The only caveat now is that as of Python 2.7, DeprecationWarnings are ignored by default, and the flag to enable them (python -Wall or python -Wd) is not easily available in ipython, one has to do

python -Wall `which ipython`

@dmcdougall
Copy link
Member

@ivanov Awesome. Thanks. Regarding the DeprecationWarning, this seems relevant. In particular on the last paragraph of that section:

You can re-enable display of DeprecationWarning messages by running Python with the -Wdefault (short form: -Wd) switch, or by setting the PYTHONWARNINGS environment variable to "default" (or "d") before running Python. Python code can also re-enable them by calling warnings.simplefilter('default').

Should we add a warnings.simplefilter('default') to turn on the DeprecationWarning for mpl.py? I believe it's important for the user to see what API calls will break in the future, contrary to what that web page claims.

@ivanov
Copy link
Member Author

ivanov commented Dec 5, 2012

@dmcdougall thanks for that link, it seems the decision to silence DeperecationWarning was made without considering the fact that developers would want to use it to clearly signal upcoming changes in their library.

Here are a few options that I see, will seek broader feedback on mpl-devel list:

A) turn on warnings.simplefilter('default') in mpl.py, so only users of this module would see it

I don't think we want to turn on warnings.simplefilter('default') permanently on import of this module, since what may likely follow would be other DeperecationWarnings from other places. These, in turn, would be silenced again as soon as the user stopped importing matplotlib.mpl.

B) turn on warnings.simplefilter('default') in mpl.py, and turn them off again (for just this one warning)

This seems a bit inelegant, and does not solve our otherwise silenced DeperecationWarning`s in other places, there are about 40 others right now.

C) turn on warnings.simplefilter('default') in matplotlib's __init__.py

This gets all of matplotlib's DeperecationWarnings 'activated', but it will also activate them from other modules the user might be working with.

I think, overall, we should not be in the business of taking control away form the user - I'd be annoyed as a user to have to disable something matplotlib did on my behalf, especially when default python mechanisms for disabling or enabling warnings from the command line would be ignored after this.

D) make a MatplotlibDeperecationWarning class that inherits from UserWarning - and use it everywhere we currently use DeperecationWarning.

UserWarnings are still enabled by default, so these will make their way in front the eyeballs of our users. PR #1565 has an implemntation of this approach, it seems like the right one to me.

@mdboom
Copy link
Member

mdboom commented Dec 6, 2012

I agree with the suggestion to move to MatplotlibDeprecationWarning. Seems the cleanest thing to do. Be prepared for an onslaught of new warnings presented to our users ;) Maybe we want to add a "deprecations" page to the docs with some advice on how to fix each one? (Or maybe this could be a subsection of api_changes.rst?)

@ivanov
Copy link
Member Author

ivanov commented Dec 14, 2012

this can be merged after #1597, since it now utilizes the MatplotlibDeprecationWarning class. Also added documentation to API Changes about this deprecation, at @mdboom's suggestion

@dmcdougall
Copy link
Member

@ivanov The deprecations PR was merged, so this will need rebasing. However, before you do rebase it, we should think about when this should be deprecated. If we deprecate now (as in, for a 1.2.1 release), we run the risk of the deprecation not getting enough visibility if 1.3 rolls around quicker than expected. If we deprecate in 1.3 (which is what this PR is doing currently), mpl.py won't be removed until 1.4/2.0, and that might be a while. There's no 'correct' approach here, just two different ones with different outcomes.

I have never used mpl.py, so I'm not sure I have the necessary experience to give an objective suggestion.

@ivanov
Copy link
Member Author

ivanov commented Dec 19, 2012

Ok, thanks for the reminder to rebase. I don't see a reason to rush with removing this, even if we deprecate in 1.2.1, we probably shouldn't remove until 1.4 - I just wanted to see this go the way of the dodo at some point, and this will be a first important step toward that.

@dmcdougall
Copy link
Member

Actually, come to think of it, 1.2.1 is a bugfix release. Deprecating mpl.py doesn't fix a bug, so I think master is more appropriate, which is what you've done here.

@ivanov
Copy link
Member Author

ivanov commented Dec 19, 2012

oh yeah, I think I also had that thinking to begin with...

@efiring
Copy link
Member

efiring commented Jan 10, 2013

@ivanov, It looks like this unfortunately needs rebasing, after which it can be merged. I don't quite understand the bit about finance, but I trust you do and have handled it correctly.

@NelleV
Copy link
Member

NelleV commented Jan 11, 2013

Just a tiny nitpick: could you add the version of matplotlib when the removal of mpl will take place? I find this very useful both for users and developpers (the latter then being able to git grep the code in order to remove all deprecated stuff just before the release, instead of going through the git blame process to find out at which date the methods/module have been deprecated).

@dmcdougall
Copy link
Member

@NelleV Where do you think it should go? In the warning message?

@NelleV
Copy link
Member

NelleV commented Jan 16, 2013

@dmcdougall Yes, this way the users see it.

Numpydoc also recommands adding it to the docstring as the following example:

.. note:: Deprecated in Numpy 1.6
          `ndobj_old` will be removed in Numpy 2.0,

@dmcdougall
Copy link
Member

In the interest of getting this merged, I'm going to fork this branch to address @NelleV's concerns.

@dmcdougall dmcdougall mentioned this pull request Jan 16, 2013
@dmcdougall
Copy link
Member

See #1670.

@dmcdougall
Copy link
Member

Resolved in #1670.

@dmcdougall dmcdougall closed this Jan 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants