NeuralEnsemble / ephyviewer

Simple viewers for ephys signals, events, video and more
https://ephyviewer.readthedocs.io
MIT License
51 stars 13 forks source link

Fix failing tests #155

Closed jpgill86 closed 3 years ago

jpgill86 commented 3 years ago

Hi @samuelgarcia,

This branch off master passes tests with pyqtgraph==0.11.1 in requirements.txt.

PyQtGraph 0.12.0 was released in March, after my most recent changes to ephyviewer, which had passing tests, and before your changes for Neo and SpikeInterface, which had failing tests.

This PR is marked as a draft because it doesn't actually fix anything yet. Tests pass locally on my machine with the latest PyQtGraph (0.12.2), so it's not yet obvious where the problem is.

If you are desperate, you could try a release with pyqtgraph pinned to 0.11.1, or perhaps pyqtgraph>=0.10.0,<0.12 (you should test that first, of course). I'm going to keep looking into this.

jpgill86 commented 3 years ago

So far I've noticed these tests are consistently failing:

jpgill86 commented 3 years ago

Aha:

ephyviewer/tests/test_videoviewer.py::test_videoviewer FAILED            [100%]

=================================== FAILURES ===================================
_______________________________ test_videoviewer _______________________________

interactive = False

    def test_videoviewer(interactive=False):
>       source = make_fake_video_source()

ephyviewer/tests/test_videoviewer.py:8: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
ephyviewer/tests/testing_tools.py:112: in make_fake_video_source
    make_video_file(filename, )
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

filename = 'video0.avi', codec = 'mpeg4', rate = 25.0

    def make_video_file(filename, codec='mpeg4', rate=25.): # mpeg4 mjpeg libx264
        print('make_video_file', filename)
        name=filename.replace('.avi', '')
        import av

        w, h = 800, 600

        output = av.open(filename, 'w')
        stream = output.add_stream(codec, rate)
        #~ stream.bit_rate = 8000000
        #~ stream.pix_fmt = 'yuv420p'
        stream.pix_fmt = 'yuv420p'
        stream.height = h
        stream.width = w

        duration = 10.

        #~ one_img = np.ones((h, w, 3), dtype='u1')
        #~ fig, ax = plt.subplots(figsize=(w/100, h/100), dpi=100,)
        fig = plt.figure(figsize=(w/100, h/100), dpi=100,)
        #~ line, = ax.plot([0], [0], marker='o', markersize=50, color='m')
        #~ ax.set_xlim(-1,1)
        #~ ax.set_ylim(-1,1)
        for i in range(int(duration*rate)):
            fig.clear()
            #~ one_img += np.random.randint(low=0, high=255, size=(h, w, 3)).astype('u1')
            text = '{} - frame {:5} - time {:6.3f}s'.format(name, i, i/rate)
            fig.text(.5, .5, text, ha='center', va='center')
            #~ ax.set_title()
            #~ line.set_markersize(i)
            fig.canvas.draw()
>           one_img = np.frombuffer(fig.canvas.tostring_rgb(), dtype='u1').reshape(h,w,3)
E           ValueError: cannot reshape array of size 1557504 into shape (600,800,3)

ephyviewer/tests/testing_tools.py:86: ValueError
jpgill86 commented 3 years ago

OK, that's confirmation that tests pass with PyQtGraph 0.12.0, as long as these tests are commented out:

Next I'll confirm that the newest PyQtGraph (0.12.2) also works.

jpgill86 commented 3 years ago

Good. All PyQtGraph 0.12.x seem to be equivalent with respect to this issue.

So it seems that make_fake_video_source is failing due to np.frombuffer returning an array of the wrong shape. I'm puzzled by two things:

  1. It works on my machine with the same versions of Numpy, Matplotlib, and PyAV.
  2. This shouldn't have anything to do with PyQtGraph.
jpgill86 commented 3 years ago

In an empty Python 3.8 environment:

pip install numpy matplotlib

# Successfully installed cycler-0.10.0 kiwisolver-1.3.2 matplotlib-3.4.3 numpy-1.21.2 pillow-8.3.2 pyparsing-2.4.7 python-dateutil-2.8.2 six-1.16.0

Then run this minimal test code (based on make_video_file) in the Python interpreter:

import numpy as np
import matplotlib.pyplot as plt

w, h = 800, 600
fig = plt.figure(figsize=(w/100, h/100), dpi=100)
fig.canvas.draw()

size = np.frombuffer(fig.canvas.tostring_rgb(), dtype='u1').size
assert size == w*h*3, f'array had unexpected size {size} instead of {w*h*3}'

The assertion passes.

Then install the latest PyQt5:

pip install PyQt5

# Successfully installed PyQt5-5.15.4 PyQt5-Qt5-5.15.2 PyQt5-sip-12.9.0

Repeating the test code, the assertion fails:

AssertionError: array had unexpected size 2250000 instead of 1440000

Although the test code does not invoke PyQt5 directly, I am guessing that the default backend for matplotlib changed once PyQt5 was installed.

All this suggests that the newest PyQt5 comes with some changes that break this bit of code. There's just one problem with this explanation: The new PyQt5 was installed in ephyviewer's GitHub Actions test environment both when it failed with PyQtGraph 0.12.0 and when it succeeded with PyQtGraph 0.11.1. Furthermore, when it failed, the array's size was instead 1557504. Perhaps there is a difference in details between the Windows build that I'm using and the Linux build running on GitHub Actions.

Maybe there is a better way to write this one line.

jpgill86 commented 3 years ago

Although the test code does not invoke PyQt5 directly, I am guessing that the default backend for matplotlib changed once PyQt5 was installed.

I was right on this point. Before PyQt5, type(fig.canvas) gives matplotlib.backends.backend_tkagg.FigureCanvasTkAgg. After installing PyQt5, it gives matplotlib.backends.backend_qt5agg.FigureCanvasQTAgg.

jpgill86 commented 3 years ago

In 7f87615, I replaced the troublesome line in make_video_file,

https://github.com/NeuralEnsemble/ephyviewer/blob/469770eb7b5840523102b72e62f2d4fbe2bcc2ca/ephyviewer/tests/testing_tools.py#L86

with something else:

one_img = np.zeros((h, w, 3), 'u1')  # TODO FIX -- this is a hack for testing -- it creates a uniform black image

Now all the tests pass with the latest PyQtGraph.

So, it seems that fig.canvas.tostring_rgb() returns something different than we are used to with the latest PyQt5 (5.15.4). We should change that line to work with the new PyQt5.

I'm still bothered by the fact that 152321f passed its tests, since it used the new PyQt5 too...

jpgill86 commented 3 years ago

The problem is the figure size. The DPI is set incorrectly with the newest PyQt5 5.15.4. The result of tostring_rgb() is then not of the correct length.

Without PyQt5 installed, or with a slightly old PyQt5 5.12.3:

import matplotlib.pyplot as plt
plt.figure(dpi=100).dpi

# returns 100 as expected

With the newest PyQt5 5.15.4:

import matplotlib.pyplot as plt
plt.figure(dpi=100).dpi

# returns 125.0

I think this might be a bug in Matplotlib. I'm running Matplotlib version 3.4.3 locally, the latest version.

jpgill86 commented 3 years ago

After testing older and older Matplotlib versions (3.4.3, 3.4.2, 3.4.1, 3.4.0, and finally 3.3.4), I can say that the problem of the dpi argument not being respected by plt.figure was introduced in 3.4.0.

Edit: This bug exists only if the newer PyQt5 is also installed. With the older version, Matplotlib 3.4.x seems fine....

jpgill86 commented 3 years ago

A pre-release version 3.5.0b1 of Matplotlib seems to have fixed the problem.

jpgill86 commented 3 years ago

Alright, it's fixed. I found a good workaround for the Matplotlib bug.

jpgill86 commented 3 years ago

PySide2 is working too.

I'm going to merge into master.

samuelgarcia commented 3 years ago

Yeaaaaaaaaaaah. You are wizard! Please come to Lyon you deserve a bootle of wine!!