Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

WIP: Deprecation of the cbook module #1647

Closed
wants to merge 16 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

NelleV commented Jan 9, 2013

Most of the methods in cbook are only meant for internal use. This PR deprecates the cbook module in favor of a new one which will not be mentionned in the user documentation. Moreover many of the methods present in the current cbook module are not used in matplotlib anymore. Hence, deprecating the module will help removing unused methods in the future.

I've proceeded into two steps:

  1. As suggested by @mdboom, I deprecated the module cbook, and move the methods to a private module _cbook
  2. I then moved all the used methods to a new module utils, and updated the code, documentation, tests and examples to reflect that change.

Hence we now have two new submodules: _cbook, which contains methods that aren't used in matplotlib, and utils containing methods used in matplotlib.

Contributor

NelleV commented Jan 9, 2013

This PR is built on #1643. Hence #1643 needs to be merged before this one.

@WeatherGod WeatherGod commented on the diff Jan 9, 2013

lib/matplotlib/utils/_cbook.py
+"""
+from __future__ import print_function
+
+import datetime
+import glob
+import gzip
+import os
+import re
+import sys
+import threading
+import time
+
+import matplotlib
+
+import numpy as np
+from matplotlib.utils import *
@WeatherGod

WeatherGod Jan 9, 2013

Member

This is extremely bad style. I would rather see "import matplotlib.utils as mutils".

@NelleV

NelleV Jan 9, 2013

Contributor

I'd rather not have to change the methods in this module, (which is meant to be removed at some point). I could import one by one the methods I need from utils (I think I need about half a dozen). I'll update this.

@mdboom

mdboom Jan 9, 2013

Owner

I think in this case, this import * makes a lot of sense. In the case where one module is basically providing a compatibility layer around another (as here) it's a completely valid usage, IMHO.

@NelleV

NelleV Jan 9, 2013

Contributor

I actually did an import * for a backward compatibility reason: I could remove it, and replace by 4 specific imports, and do an from utils import * in the cbook module for backward compatibility.

@WeatherGod

WeatherGod Jan 9, 2013

Member

Right, of course. I must have been on auto-pilot at that moment.

Owner

mdboom commented Jan 9, 2013

I think perhaps you misunderstood what I was suggesting. We need to keep from matplotlib import cbook working for any users who may be using stuff in there (and a quick search on ohloh reveals projects that are).

I suggested:

  1. Moving any internally-used stuff to _cbook.py (though as you've done here, that could just as easily have been utils -- though if we want that to be private it should be designated as such as well)

  2. Have cbook.py do from _cbook import *

  3. Include any non-internally used methods in cbook with deprecation warnings.

I'm open to other suggestions about how to maintain matplotlib.cbook, but unfortunately I think this PR as it stands just pulls the rug out from under people who may currently be using it.

Member

WeatherGod commented Jan 9, 2013

I am ambivalent about this PR. I just don't see the point in doing this. In discussions past, we were planning to make cbook.py more prominent to make both the users and the mpl devs more aware of its existance. We were finding that developers, like myself, were re-inventing the wheel because we didn't know what was in cbook (because it wasn't well documented on the website). We changed that a couple of versions ago so that it would be in the docs.

Now, I would admit that it needs significant cleanup because some of these cbook functions were intended to work around limitations in older versions of python and numpy, and so their use can be of very limited value now. But I see no reason to not just simply judiciously put those functions on the normal deprecation path.

Contributor

NelleV commented Jan 9, 2013

I'm not sure how this is different from what I implement: the cbook module now raises a deprecation warning when loaded, but has all the methods it used to. I've separated internally used methods and unused methods in respectively utils and _cbook, to simplify the removal of unused internal methods. All backward compatibility is (for now) kept.

Contributor

NelleV commented Jan 9, 2013

@WeatherGod I'm completely +1 on having the cbook module more prominent for developpers. I just don't like in general the idea of keeping unused code. I also don't think most of the methods in this module should be "visible" outside of matplotlib, as it makes maintenance and refactoring harder.

Right now, many methods are not used (around 600loc, so a third of the current cbook module), and some are very sparsely used in matplotlib (once or twice), or are reimplemented in other modules. By uniformising the module (including the naming, such as, having all validation methods named "is_something"), it should be easier for developpers to understand why and how the module works.

Member

WeatherGod commented Jan 9, 2013

Just a heads-up, when the maintenance branch gets merged into master again, you will want to re-base and double-check mplot3d. I recently had to fix a bug where I forgot to import cbook (or something like that) in axes3d.py.

Owner

mdboom commented Jan 9, 2013

@NelleV: Perhaps you juts forgot to commit a file? This branch has no cbook.py at all, so from matplotlib import cbook fails.

Contributor

NelleV commented Jan 9, 2013

@mdboom Indeed, I had removed it inadvertently... Sorry about that.

I had a quick look at what people used from matplotlib.cbook on ohloh:

  • some are false positive (they mention matplotlib, and have a cbook module) (GromacsWrapper)
  • some projects ship part of matplotlib with them (scikits.image, Einstein-NTE's einstein)
  • some projects import the module, but don't really use it (they import enumerate from it, which is in the stdlib, or import it and don't use it).
  • some projects import, and use the same methods that are currently being used in matplotlib (is_string_like, is_scalar_like, Bunch, iterable, and get_sample_data)

I don't think any uses methods that are in _cbook in this PR, but I may by mistaken.

Amongst the method from cbook I have seen being used in the projects I had a look at, I don't think any are going to be removed any time soon, as they are validation of input methods.

Owner

efiring commented Feb 18, 2013

I don't see that we gain enough by essentially renaming cbook to utils to justify all this churn. If anything is still to be done along these lines, I think it should be more incremental, and will need a fresh PR.

@efiring efiring closed this Feb 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment