craffel / mir_eval

Evaluation functions for music/audio information retrieval/signal processing algorithms.
MIT License
604 stars 112 forks source link

isolate pyplot import and side-effects #278

Closed bmcfee closed 6 years ago

bmcfee commented 6 years ago

(Note: we recently fixed this over in librosa here.)

The issue is that importing matplotlib.pyplot triggers the backend selector, which has side-effects (including occasional memory leaks). But we don't always need to import pyplot in the display module. If the user is working with the object API (as opposed to the stateful pyplot API), then we can avoid this import entirely.

In fact, the only place the pyplot import is used is within our axes getter, and even then, only when the axes are not provided by the user. It's an easy fix to move the import to within that conditional block, and avoid any side-effects until they are absolutely necessary.

craffel commented 6 years ago

Since the only call to plt.anything is plt.gcf, could we just reproduce the contents of plt.gcf instead of importing pyplot at all? Would look like this: https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/pyplot.py#L565 It would involve importing matplotlib._pylab_helpers (or figuring some equivalent way to do this), which I assume doesn't have the same side effects.

bmcfee commented 6 years ago

Since the only call to plt.anything is plt.gcf, could we just reproduce the contents of plt.gcf instead of importing pyplot at all?

No, I think we want the import here. The idea is that if a user does not provide axes or a figure object, then they must be using the pyplot API. If the call into our display code is the first pyplot thing the user has done, then no backend will have yet been selected. However, we really want that to happen before executing any draw commands so that the state is updated appropriately. (This solution was arrived at after conferring with the mpl devs in the context of librosa, but I think the general idea carries over.)

It's also bad form to access other packages' internals, so there's that.

craffel commented 6 years ago

Fair enough.