Random test failures in Legent tests (1.5.0rc2) #5185
Comments
|
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 |
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) |
|
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 Something really fishy is going on, the legend is a Also, the failing test was added in #1640 which looks like it was also the last time someone was poking around in the |
|
I ran across this problem today too. The issue is that In the documentation for 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: 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 |
|
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 Here is the main issue - in 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 for (size_t i = 0; i < num_bboxes; ++i) {loop does not get executed and it returns a count of zero. With 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 |
|
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 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++ 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 |
|
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 |
|
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). |
Check the dimensions on arrays passed to C++ to ensure they are what we expect.
|
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 |
Should catch issues like matplotlib#5185 while not adding a big perf penalty
|
@jkseppan: Thanks for confirming my hunch about the slowness of bounds checking in the inner loop. |
Check the dimensions on arrays passed to C++ to ensure they are what we expect.
On my 10.11 OSX machine I am seeing random failures in legent tests such as
This happens randomly for various of the tests with both python 2 and 3. I seem to be able to reproduce it outside the test suite so I don't think it is related to race conditions in the test code.
The text was updated successfully, but these errors were encountered: