8000 Qt embedding example: Separate drawing and data retrieval timers by doronbehar · Pull Request #28589 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Qt embedding example: Separate drawing and data retrieval timers #28589

8000 New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

doronbehar
Copy link
Contributor

PR summary

While developing a Qt application that had to retrieve data as quickly as possible from a measurement instrument, and display that in multiple plots, I encountered an issue with my 1ms timers causing the application to not respond to events, and essentially making the app stuck and unusable. ChatGPT helped me to figure out I need to process the events in my timer timeout function. This has bugged me for a week or so, and this is not trivial to figure out by oneself IMO. Hence this PR. See also pyqtgraph/pyqtgraph#3092.

PR checklist

  • [N/A] "closes #0000" is in the body of the PR description to link the related issue
  • [N/a] new and changed code is tested - it is hard to test this, as the issue is present mostly on Windows and on slow desktops.
  • Plotting related features are demonstrated in an example
  • [N/A] New Features and API Changes are noted with a directive and release note
  • Documentation complies with general and docstring guidelines

@github-actions github-actions bot added the Documentation: examples files in galleries/examples label Jul 18, 2024
@doronbehar doronbehar force-pushed the qt_api_processEvents branch 2 times, most recently from d2dd466 to e632e9e Compare July 18, 2024 09:47
@tacaswell
Copy link
Member

Given that we are fully embedding in Qt in this example we should be trusting it to manage it's event queue and not "helping" (this seems very similar to trying to "help" the garbage collector which ended badly for us). It would be better to sort out if we are blocking the event loop someplace and fix that.

The timer here in

class TimerQT(TimerBase):
"""Subclass of `.TimerBase` using QTimer events."""
def __init__(self, *args, **kwargs):
# Create a new timer and connect the timeout() signal to the
# _on_timer method.
self._timer = QtCore.QTimer()
self._timer.timeout.connect(self._on_timer)
super().__init__(*args, **kwargs)
def __del__(self):
# The check for deletedness is needed to avoid an error at animation
# shutdown with PySide2.
if not _isdeleted(self._timer):
self._timer_stop()
def _timer_set_single_shot(self):
self._timer.setSingleShot(self._single)
def _timer_set_interval(self):
self._timer.setInterval(self._interval)
def _timer_start(self):
self._timer.start()
def _timer_stop(self):
self._timer.stop()
via
def new_timer(self, interval=None, callbacks=None):
"""
Create a new backend-specific subclass of `.Timer`.
This is useful for getting periodic events through the backend's native
event loop. Implemented only for backends with GUIs.
Parameters
----------
interval : int
Timer interval in milliseconds.
callbacks : list[tuple[callable, tuple, dict]]
Sequence of (func, args, kwargs) where ``func(*args, **kwargs)``
will be executed by the timer every *interval*.
Callbacks which return ``False`` or ``0`` will be removed from the
timer.
Examples
--------
>>> timer = fig.canvas.new_timer(callbacks=[(f1, (1,), {'a': 3})])
"""
return self._timer_cls(interval=interval, callbacks=callbacks)
and
class FigureCanvasQT(FigureCanvasBase, QtWidgets.QWidget):
required_interactive_framework = "qt"
_timer_cls = TimerQT
manager_class = _api.classproperty(lambda cls: FigureManagerQT)
.

I suspect that a better fix is to change the figure.canvas.draw() to figure.canvas.draw_idle() so that the rendering cost does not happen in the timer's callback.

@doronbehar
Copy link
Contributor Author

Hmm indeed using draw_idle is much better then draw, but I'm not sure that manually running processEvents is not better. Here's a small comparison video:

matplotlib.drawing.methods.comparison.mp4

And the code:

#!/usr/bin/env python

import sys
import time

import numpy as np

from matplotlib.backends.backend_qtagg import FigureCanvas
from matplotlib.backends.backend_qtagg import \
    NavigationToolbar2QT as NavigationToolbar
from matplotlib.backends.qt_compat import QtWidgets, QtCore
from matplotlib.figure import Figure


class ApplicationWindow(QtWidgets.QMainWindow):
    def __init__(self, method):
        super().__init__()
        self._main = QtWidgets.QWidget()
        self.setCentralWidget(self._main)
        self.method = method
        self.setWindowTitle(method)
        layout = QtWidgets.QVBoxLayout(self._main)

        static_canvas = FigureCanvas(Figure(figsize=(5, 3)))
        # Ideally one would use self.addToolBar here, but it is slightly
        # incompatible between PyQt6 and other bindings, so we just add the
        # toolbar as a plain widget instead.
        layout.addWidget(NavigationToolbar(static_canvas, self))
        layout.addWidget(static_canvas)


        self._static_ax = static_canvas.figure.subplots()
        t = np.linspace(0, 10, 501)
        self._static_ax.plot(t, np.tan(t), ".")

        self.t = np.linspace(0, 10, 10001)

        dynamic_canvas1 = FigureCanvas(Figure(figsize=(5, 3)))
        layout.addWidget(dynamic_canvas1)
        layout.addWidget(NavigationToolbar(dynamic_canvas1, self))
        self._dynamic_ax1 = dynamic_canvas1.figure.subplots()
        # Set up a Line2D.
        self._line1, = self._dynamic_ax1.plot(t, np.sin(t + time.time()))

        dynamic_canvas2 = FigureCanvas(Figure(figsize=(5, 3)))
        layout.addWidget(dynamic_canvas2)
        layout.addWidget(NavigationToolbar(dynamic_canvas2, self))
        self._dynamic_ax2 = dynamic_canvas2.figure.subplots()
        # Set up a Line2D.
        self._line2, = self._dynamic_ax2.plot(t, np.sin(t + time.time()))

        self._timer = QtCore.QTimer()
        self._timer.timeout.connect(self._update_canvas)
        self._timer.start(1)

    def _update_canvas(self):
        # Shift the sinusoid as a function of time.
        self._line1.set_data(self.t, np.sin(self.t + time.time()))
        self._line2.set_data(self.t, np.sin(self.t - time.time()))
        if self.method == "draw":
            self._line1.figure.canvas.draw()
            self._line2.figure.canvas.draw()
        elif self.method == "draw_idle":
            self._line1.figure.canvas.draw_idle()
            self._line2.figure.canvas.draw_idle()
        elif self.method == "processEvents":
            self._line1.figure.canvas.draw()
            self._line2.figure.canvas.draw()
            qapp.processEvents()


if __name__ == "__main__":
    # Check whether there is already a running QApplication (e.g., if running
    # from an IDE).
    qapp = QtWidgets.QApplication.instance()
    if not qapp:
        qapp = QtWidgets.QApplication(sys.argv)

    app = ApplicationWindow(sys.argv[1])
    app.show()
    app.activateWindow()
    app.raise_()
    qapp.exec()

Note that this issue is significant (at least for me) only on Windows.

@tacaswell
Copy link
Member

One of my favorite jokes is: Both junior engineers and senior engineers take the same short-cuts, but the senior engineers know to feel guilty about it.

You are asking the timer to run at at 1kHz (my understanding is .start(1) sets a timer with a 1ms period) which is faster than your screen can update and definitely faster than you can see!

My understanding of processEvents is that it is for cases where you are intentionally preventing the main eventloop from running (like putting in some slow compute) and it is more practical to manually spin the event loop rather than switch to using (q)threads to push push the compute off the event loop. Maybe also in modal dialogs (it has been a while since I worked deeply with Qt). That is not the case here so we should not have our docs show light abuse of the API. If my understanding is wrong, can you provide a reference (written by an actual human) otherwise?

@doronbehar
Copy link
Contributor Author

OK I got convinced that using processEvents is an abuse of the API, and also the fact that there is no reason to update the GUI in a frequency faster then our eye can observe requires too much resources and that such a high frequency is not really needed for the GUI to appear to change smoothly.

Hence I modified the PR such that it uses two timers with different frequencies - the high frequency timer for updating the data and the slow (50Hz) timer for drawing the canvas. Also the comments now explain the topic much better for beginner Qt users.

@doronbehar doronbehar force-pushed the qt_api_processEvents branch from e632e9e to 8b89c80 Compare July 22, 2024 12:32
@doronbehar doronbehar changed the title Qt embedding example: processEvents, to make sure GUI is smooth Qt embedding example: Separate drawing and data retrieval timers Jul 22, 2024
@doronbehar doronbehar force-pushed the qt_api_processEvents branch from 8b89c80 to 0f92e8c Compare July 22, 2024 12:33
self.drawingTimer.start()

def _update_data(self):
self.xdata = np.linspace(0, 10, 101)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To check my understanding, because these are being run by timmers, that means they are run atomically on the event loop so we do not need to worry about locking between this method and _update_canvas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To check my understanding, because these are being run by timmers, that means they are run atomically on the event loop so we do not need to worry about locking between this method and _update_canvas?

I'm not sure what do you mean by locking, are you talking about potential issues of xdata not in sync with ydata or such?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about the sequence of things where:

  • you set self.xdata
  • self._line.set_data(self.xdata, self.ydata) runs
  • you set self.ydata

In this particular example, if we were losing that race it would be OK because the xdata is always the same (important from a "correct plot" point of view) and the same length (important from a "the code will run without raising" point of view). If there are threads involved the above sequence is possible as with non-free threading Python (aka all released versions) the interpreter can drop the GIL after any bytecode evaluation and then any thread can be the next one to run (and in the future with free-threading builds of Python it is just the YOLO land).

However, if the calls to these methods are serialized by the event loop (it runs the one at a time in the order they were put on the queue) then we are safe because Qt is providing the level of locking we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I see what you mean. Currently the dataTimer starts (with .start()) before the drawingTimer starts, and also the timeout of the 1st timer for sure is reached before the timeout of the 2nd timer (due to the different intervals), so I am pretty sure we can count on that.

Not only that, even if both timers had the same interval, the drawingTimer's callback that uses self._line.set_data(self.xdata, self.ydata) always has a self.{x,y}data attributes available, because I ran manually the self._update_data method before I create the line with self._dynamic_ax.plot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the case where :

import random
self.xdata = np.linspace(0, 10, random.randint(100, 150))

Which callback run first or if the attributes exist is not the issue, the chance of them running concurrently is the problem!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the case where :

import random
self.xdata = np.linspace(0, 10, random.randint(100, 150))

Which callback run first or if the attributes exist is not the issue, the chance of them running concurrently is the problem!

Hmm I'm pretty sure that the timers' callbacks run concurrently, and they block the event loop - that's my main conclusion of the investigation I performed on the "stuck" issue I experienced. In the case you presented, at the same callback that sets self.xdata, self.ydata should be set as well, to make sure they have the same size, and then when the blocking callback of the drawing happens - they will still have the same size.

Perhaps it is worth explaining in a comment? Or perhaps I will set self.xdata in the _update_data callback to the same value, although it is not really needed, just for the sake of the example?

@tacaswell tacaswell added this to the v3.10.0 milestone Jul 22, 2024
Copy link
Member
@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I think this does add some complexity to example, but I think it is complexity the represents problems users who go down this route are likely to actually face (and the other option in QThreads).

In the previous version of this example, if the timer interval would
have been simply decreased to 1ms, the GUI could have appeared as
stuck on some platforms / (slow) machines, because the event loop didn't
find the time to respond to external events such as the user dragging
the window etc. This change does 3 things:

- Puts the data to plot in the self.{x,y}data attributes.
- Separates the timer that updates the self.{x,y}data from the timer
  that updates the canvas
- Explains much better the reasoning for the timers' intervals choices
  in comments, as well as explaining why the timers are attributed to
  self, although they are not used by other methods of the class.
- Use the asynchronous draw_idle() function to further guarantee that
  the drawing won't be blocking.

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@doronbehar doronbehar force-pushed the qt_api_processEvents branch from 28af910 to b86e35e Compare August 3, 2024 17:12
@doronbehar
Copy link
Contributor Author

Thanks for the suggestions @QuLogic . All applied :)

Copy link
Member
@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the liberty of renaming _update_data -> _update_ydata.

@timhoffm timhoffm merged commit d239b36 into matplotlib:main Aug 5, 2024
19 checks passed
@doronbehar
Copy link
Contributor Author

I took the liberty of renaming _update_data -> _update_ydata.

Sounds better to me. Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: examples files in galleries/examples
Projects
Development

Successfully merging this pull request may close these issues.

4 participants
0