I still se this. No one else seeing it?

is this single-process or multi-process testing? I have not seen this locally:

for j in `seq 1 10`; do python tests.py matplotlib.tests.test_legend --processes=8 --process-timeout=300; done;
for j in `seq 1 10`; do python tests.py matplotlib.tests.test_legend; done;

but have only been testing on 3.5

Single process. Multiprocess is completely broken on OSX. It happens with both python 2.7 and 3.5 and I can reproduce it outside the test suite

Multiprocess is completely broken on OSX.

Do you mean in general or just our test suite? Do we have an open issue for that? It would be good to fix.

I can not reproduce this on linux + 3.5 (passed 100x times)

Our test suite. I think there is an old issue somewhere

Ok I will investigate again.

It's not exactly #3314 but related to it.

Running:

from __future__ import unicode_literals

import matplotlib.pyplot as plt
import numpy as np

for i in range(100):
    fig = plt.figure()
    ax = fig.add_subplot(121)
    ax.plot(list(xrange(4)), 'o', label=1)
    ax.plot(np.linspace(4, 4.1), 'o', label='D\xe9velopp\xe9s')
    ax.plot(list(xrange(4, 1, -1)), 'o', label='__nolegend__')
    ax.legend(numpoints=1, loc=0)
    plt.savefig('test' + str(i) + '.png')
    plt.close(fig)

I see 15 failures as below (some repeated):

This is on OSX 10.11 with Homebrew python 2.7 (I can also reproduce it with 3.5)

alabaster (0.7.6)
appnope (0.1.0)
apptools (4.3.0)
astcheck (0.2.4)
astroid (1.3.8)
Babel (2.1.1)
backports.ssl-match-hostname (3.4.0.2)
bokeh (0.10.0)
bottle (0.12.8)
cairocffi (0.7.2)
certifi (2015.9.6.2)
cffi (1.2.1)
codecov (1.5.1)
colorama (0.3.3)
configobj (5.0.6)
coverage (4.0)
coveralls (1.0)
CProfileV (1.0.7)
cycler (0.9.0)
Cython (0.23.4)
decorator (4.0.4)
dill (0.2.4)
docopt (0.6.2)
docutils (0.12)
enum34 (1.0.4)
envisage (4.4.0)
Flask (0.10.1)
funcsigs (0.4)
functools32 (3.2.3.post2)
gevent (1.0.2)
gnureadline (6.3.3)
greenlet (0.4.9)
grin (1.2.1)
ipykernel (4.1.0)
ipyparallel (4.0.2)
ipython (4.0.0)
ipython-genutils (0.1.0)
itsdangerous (0.24)
Jinja2 (2.8)
jsonschema (2.5.1)
jupyter-client (4.1.1)
jupyter-console (4.0.3)
jupyter-core (4.0.6)
llvmlite (0.7.0)
logilab-common (1.1.0)
Mako (1.0.2)
Markdown (2.6.2)
MarkupSafe (0.23)
matplotlib (1.5.0rc2+38.g0cb18e0)
mayavi (4.4.4.dev0)
mistune (0.7.1)
mock (1.3.0)
mpldatacursor (0.6.1)
mplstereonet (0.5)
nbconvert (4.0.0)
nbformat (4.0.1)
nose (1.3.7)
numba (0.21.0)
numpy (1.10.1)
ordereddict (1.1)
pandas (0.17.0)
pandocfilters (1.2.4)
path.py (8.1.2)
pbr (1.8.1)
pdb (0.1)
pep8 (1.6.2)
pexpect (4.0.1)
pickleshare (0.5)
Pillow (3.0.0)
pip (7.1.2)
ply (3.8)
ptyprocess (0.5)
pudb (2015.3)
py (1.4.30)
pycca (0.2.0)
pycparser (2.14)
pyface (4.5.2)
Pygments (2.0.2)
pygobject (3.18.0)
pylint (1.4.4)
pyparsing (2.0.3)
pystache (0.5.4)
pytest (2.8.2)
python-dateutil (2.4.2)
python-gnupg (0.3.8)
pytz (2015.6)
PyYAML (3.11)
pyzmq (14.7.0)
requests (2.8.1)
rollbar (0.10.1)
scipy (0.16.0)
setuptools (18.4)
simplegeneric (0.8.1)
singledispatch (3.4.0.3)
six (1.10.0)
snowballstemmer (1.2.0)
Sphinx (1.3.1)
sphinx-rtd-theme (0.1.9)
stevedore (1.9.0)
sympy (0.7.6.1)
terminado (0.5)
tornado (4.2.1)
traitlets (4.0.0)
traits (4.5.0)
traitsui (4.5.1)
urwid (1.3.0)
vboxapi (1.0)
virtualenv (13.1.2)
virtualenv-clone (0.2.6)
virtualenvwrapper (4.7.1)
websocket (0.2.1)
Werkzeug (0.10.4)
wheel (0.26.0)
wxPython (3.0.2.0)
wxPython-common (3.0.2.0)

Indeed, this is more serious than I had thought. I should have some time to look at this tomorrow.

Let me see if I can bisect it before doing anything else

I can't reproduce on either 2.7 or 3.5 using up-to-date anaconda in linux.

Bisect point to 2f5a633

That commit went in as part of #5106 and is a bit misleading as the ensure3D function was added and removed in that PR.

Something really fishy is going on, the legend is a VPacker object (as near as I can tell) which holds the text and artist and the frame is a FancyBboxPatch. It looks like the 'best' location is computed twice, once to set the size / location of the patch and once as part of the packer draw. Maybe it is (randomly?!?) deciding on a different 'best' location due to hitting it's own patch.

Also, the failing test was added in #1640 which looks like it was also the last time someone was poking around in the loc='best' code

I ran across this problem today too. The issue is that numpy.atleast_3d does not exactly do what was trying to being accomplished.

> np.array([])[:, None, None]  # effectively what ensure_3d does
array([], shape=(0, 1, 1), dtype=float64)  # looks good - three dimensional array that is empty on 1st dimension
> np.atleast_3d([])
array([], shape=(1, 0, 1), dtype=float64)  # not quite what one wants - this is not empty

In the documentation for atleast_3d:

For example, a 1-D array of shape (N,) becomes a view of shape (1, N, 1),

which forces at least one element along the first dimension

Looks like a simple fix is to revert commit 2f5a633

I'm not sure I follow:

array([], shape=(1, 0, 1), dtype=float64)  # not quite what one wants - this is not empty

Isn't that empty? It shouldn't matter where the 0 is...

I bet we have a test someplace which checks if the array is empty by checking that foo.shape[0] == 0 and if that check is in the c++ layer we can grab uninitialized data.

This looks wrong: https://github.com/matplotlib/matplotlib/blob/master/src/numpy_cpp.h#L482

    size_t size() const
    {
        return (size_t)dim(0);
    }

Shouldn't this be

    size_t size() const
    {
    if (ND == 0)
    {
        return 0;
    }

    size_t out = m_shape[0];
    for(int i = 1; i < ND; ++i)
    {
        out *= m_shape[i];
    }

    return out;

    }

Technically, they are both empty 3-d arrays but the difference is subtle. This could be fixed in _path.h as well if we wanted to go down that route to use numpy.atleast_3d.

Here is the main issue - in _path.h you have:

template <class BBoxArray>
int count_bboxes_overlapping_bbox(agg::rect_d &a, BBoxArray &bboxes)
{
    agg::rect_d b;
    int count = 0;

    if (a.x2 < a.x1) {
        std::swap(a.x1, a.x2);
    }
    if (a.y2 < a.y1) {
        std::swap(a.y1, a.y2);
    }

    size_t num_bboxes = bboxes.size();
    for (size_t i = 0; i < num_bboxes; ++i) {
        typename BBoxArray::sub_t bbox_b = bboxes[i];
        b = agg::rect_d(bbox_b(0, 0), bbox_b(0, 1), bbox_b(1, 0), bbox_b(1, 1));

        if (b.x2 < b.x1) {
            std::swap(b.x1, b.x2);
        }
        if (b.y2 < b.y1) {
            std::swap(b.y1, b.y2);
        }
        if (!((b.x2 <= a.x1) || (b.y2 <= a.y1) || (b.x1 >= a.x2) || (b.y1 >= a.y2))) {
            ++count;
        }
    }

    return count;
}

With ensure_3d the first dimension is size 0 so you can see that the

for (size_t i = 0; i < num_bboxes; ++i) {

loop does not get executed and it returns a count of zero.

With numpy.atleast_3d, the first dimension has size 1 so the the loop executes once. Inside that loop there is no check that the corresponding bbox_b is not size zero.

typename BBoxArray::sub_t bbox_b = bboxes[i];
b = agg::rect_d(bbox_b(0, 0), bbox_b(0, 1), bbox_b(1, 0), bbox_b(1, 1));

It is assumed to be a valid bbox of dimension 2 when in fact it is not. The rect_d b is just garbage at that point.

I see. This is really helpful investigation. I think it might make more sense to fix this on the C++ side (as there's still no guarantee we wouldn't hit this through some other code path).

my fix breaks a whole bunch of things as where are a whole bunch of places where we use size == a.shape[0]

13:21 $ git grep '\.size('
_backend_agg.h:        if (gc.dashes.size() == 0) {
_backend_agg.h:    size_t Noffsets = offsets.size();
_backend_agg.h:    size_t Ntransforms = transforms.size();
_backend_agg.h:    size_t Nfacecolors = facecolors.size();
_backend_agg.h:    size_t Nedgecolors = edgecolors.size();
_backend_agg.h:    size_t Nlinewidths = linewidths.size();
_backend_agg.h:    size_t Nlinestyles = std::min(linestyles.size(), N);
_backend_agg.h:    size_t Naa = antialiaseds.size();
_backend_agg.h:    if (edgecolors.size() == 0) {
_backend_agg_basic_types.h:        return dashes.size();
_contour.cpp:    assert(index >= 0 && index < static_cast<long>(_lines.size()) &&
_contour.cpp:    npy_intp dims[2] = {static_cast<npy_intp>(contour_line.size()), 2};
_contour.cpp:            npy_intp npoints = static_cast<npy_intp>(line.size() + 1);
_path.h:    size_t n = points.size();
_path.h:    for (i = 0; i < points.size(); ++i) {
_path.h:    for (i = 0; i < points.size(); ++i) {
_path.h:    size_t Npaths = paths.size();
_path.h:    size_t Noffsets = offsets.size();
_path.h:    size_t Ntransforms = std::min(transforms.size(), N);
_path.h:    size_t Npaths = paths.size();
_path.h:    size_t Ntransforms = std::min(transforms.size(), N);
_path.h:    if (polygon.size() == 0) {
_path.h:        if (polygon1.size()) {
_path.h:    size_t num_bboxes = bboxes.size();
_path_wrapper.cpp:    PyObject *pyresult = PyList_New(polygons.size());
_path_wrapper.cpp:    for (size_t i = 0; i < polygons.size(); ++i) {
_path_wrapper.cpp:        npy_intp dims[] = {(npy_intp)poly.size() + 1, 2 };
_path_wrapper.cpp:        memcpy(subresult.data(), &poly[0], sizeof(double) * poly.size() * 2);
_path_wrapper.cpp:        subresult(poly.size(), 0) = poly[0].x;
_path_wrapper.cpp:        subresult(poly.size(), 1) = poly[0].y;
_path_wrapper.cpp:    npy_intp dims[] = {(npy_intp)result.size() };
_path_wrapper.cpp:    if (result.size() > 0) {
_path_wrapper.cpp:        memcpy(pyresult.data(), &result[0], result.size() * sizeof(int));
_path_wrapper.cpp:    size_t length = codes.size();
ft2font.cpp:    for (size_t i = 0; i < glyphs.size(); i++) {
ft2font.cpp:    for (size_t i = 0; i < glyphs.size(); i++) {
ft2font.cpp:    for (size_t n = 0; n < glyphs.size(); n++) {
ft2font.cpp:    for (size_t n = 0; n < glyphs.size(); n++) {
ft2font.cpp:    if (glyphInd >= glyphs.size()) {
ft2font.h:        return glyphs.size() - 1;
ft2font.h:        return glyphs.size();
ft2font_wrapper.cpp:    npy_intp dims[] = {(npy_intp)xys.size() / 2, 2 };

For expediency I think we should merge #5241 for 1.5 and fix the c++ layer for 2.1 unless I am over-estimating the effort to fix the c++ side.

Just going to give my braindump -- might not have a chance to work this into a proper PR until tomorrow. @tacaswell's suggestion is where I think it should be fixed, but not quite right. Part of the confusion is that it is the implementation of C++ size, which by convention in the C++ STL should be the size of the outermost dimension, and numpy.array.size means "the total number of elements in the array". I don't think we should change the definition of what size means on the C++ side. But it should do something like "if any element in shape is 0, return 0, else return shape[0]".

EDIT: I think we just posted at the same time. Anyway, I think what I'm suggesting should hopefully fix the bug without breaking everything.

Do you just want to check if the array is empty (any dimension is size zero) and bypass the counting loop or do you want to check that each bbox is correct size (1, 1) and incur the penalty on each iteration through the loop?

Each bbox should be the same size, so you'd only have to check once outside of the loop. But I do agree that would be better.

ah, the numpy vs stl conventions make sense.

Special casing when there is a zero in one of the higher dims smells bad. I can not think of a use-case off the top of my head, but being able to iterate through the inner arrays of a (5, 0, 5) shaped array seems like something you should be allowed to do.

Yeah, you're probably right. I guess I'm coming around to fixes like #5245 being the right ones. But we should endeavor to fix those everywhere if we can.

How about bounds checking in array_view_accessors? If it turns out to be slow, make it optional and only turn it on for tests.

I could try bounds checking in the inner loop like that, but I do fear it would be too slow (I can always check I suppose). The solution I'm working on now bounds checks the dimensions of the array once when wrapped in the C++ wrapper. It won't catch all cases where the array can be read out of bounds, but it will catch cases where the array doesn't match the expected shape (and in most cases, there is an expected shape).

Yes, bounds checking in the inner loops makes the code really slow. FWIW, I tried that and left the tests running, and all test failures it caused were from count_bboxes_overlapping_bbox.

@jkseppan: Thanks for confirming my hunch about the slowness of bounds checking in the inner loop.