Use the upstream pyqtgraph

[JamesWrigley]

Previously, EXtra-foam was using a modified internal copy of pyqtgraph. For the sake of ease-of-maintenance, we decided to remove this and use upstream pyqtgraph instead (currently a commit from master). That's all this PR does, functionality is left exactly the same (with the possible exception of the crosshair widget in the azimuthal integration window, which looks different [see 65f8d3c]).

The main benefit of the internal copy was plotting performance, and in most cases upstream is still slower:

Plot type	Benchmark data size	Old FPS	New FPS
Scatter	5000	80	26
Line	5000	80	40
Bar	300	120	67
Error bar	500	93	60
Image	1024x1280	33	40

I benchmarked this with the scripts in benchmarks/gui/ on my laptop, fullscreen window on a 2K monitor. TL;DR: everything but images is way slower. So, should we still do this? IMHO yes, because:

• I do not want to have to maintain our own pyqtgraph...
• I don't think the performance is a deal-breaker, it's rare that I see fullscreen plots with thousands of points.
• Having said that, such large plots are definitely still possible, e.g. plotting something vs time. But here we still have options:
  • Use a TimedPlotWidgetF to only update the plots every second or so.
  • Use dedicated processes for rendering: https://pyqtgraph.readthedocs.io/en/latest/widgets/remotegraphicsview.html
  • And if absolutely necessary, backport the optimizations from the old version.

I would be interested in knowing if there's another reason not to merge I haven't thought of, or cases where plotting (excluding images) was a bottleneck.

(I'd also suggest reviewing each commit one-by-one, they're quite atomic and it'll make more sense)

[lgtm-com]

This pull request introduces 3 alerts and fixes 141 when merging ea40542 into fd55de3 - view on LGTM.com

new alerts:

• 2 for Module is imported with 'import' and 'import from'
• 1 for Unused import

fixed alerts:

• 34 for Unused import
• 28 for Except block handles 'BaseException'
• 27 for Unused local variable
• 10 for Testing equality to None
• 9 for 'import *' may pollute namespace
• 8 for Modification of dictionary returned by locals()
• 5 for `__eq__` not overridden when adding attributes
• 3 for `__init__` method calls overridden method
• 2 for Deprecated slice method
• 2 for Modification of parameter with default
• 2 for Variable defined multiple times
• 2 for Insecure temporary file
• 1 for Membership test with a non-container
• 1 for Module is imported more than once
• 1 for Special method has incorrect signature
• 1 for Unnecessary 'else' clause in loop
• 1 for Unreachable code
• 1 for Module is imported with 'import' and 'import from'
• 1 for Mismatch between signature and use of an overriding method
• 1 for Redundant assignment
• 1 for Nested loops with same variable

[zhujun98]

To be honest, I don't agree here. You may have seen a few use cases in the hutch, but maybe not all of them. EXtra-foam is oftentimes used in multiple big screens together. If you add up the slowdown of multiple plots, it makes a huge difference.

By the way, I am surprised the performance of image plot. Has anything been changed or maybe they have improved it a big during the past two years :-(

FYI @leguyader @jo-moeller

[ebadkamil]

I remember from my time at FXE when we were using the upstream pyqtgraph, scatter plots were super slow. FXE used to keep several plot windows open and things gets slowed down quite a bit. Jun’s modified copy of pyqtgraph improved things remarkably. I would prefer to keep it like that. Unless there are improvements in upstream.

[JamesWrigley]

Mmmm interesting, thanks for replying guys, that's very useful to know. I was hoping to avoid backporting the old optimizations, but it does look like the neatest way to get good performance across the board...

[JamesWrigley]

By the way, I am surprised the performance of image plot. Has anything been changed or maybe they have improved it a big during the past two years :-(

I can't find a reference right now, but IIRC there was a big speedup with the release of numpy 1.20.

[zhujun98]

I spent considerable time to remove a lot of craps (due to historical reason) in pyqtgraph. I also removed unneeded options which slow down the performance.

I don't think you would require too much effort to maintain it. Also, it allows to add new features quickly without wading through the pyqtgraph code base.

Before I left, @philsmt also proposed to make an internal plotting library. Since I would left soon at that time, this did not happen. I would say that it is also a good direction to go and then you can offload the burden from your should and give it to @CammilleCC :)

[zhujun98]

By the way, I am surprised the performance of image plot. Has anything been changed or maybe they have improved it a big during the past two years :-(

I can't find a reference right now, but IIRC there was a big speedup with the release of numpy 1.20.

Please make EXtra-foam faster again! MEFA, another version of MAGA 😃

[JamesWrigley]

I spent considerable time to remove a lot of craps (due to historical reason) in pyqtgraph. I also removed unneeded options which slow down the performance.

I don't think you would require too much effort to maintain it. Also, it allows to add new features quickly without wading through the pyqtgraph code base.

So, in general I'm not a fan of forking libraries for internal use; firstly because it puts the burden of maintenance (however small) on us, secondly because I don't think it's in the FOSS spirit to not contribute our improvements back to the community. As much effort as I'm sure it took to optimize and clean up pyqtgraph, I would rather those improvements go upstream :)

Before I left, @philsmt also proposed to make an internal plotting library. Since I would left soon at that time, this did not happen. I would say that it is also a good direction to go and then you can offload the burden from your should and give it to @CammilleCC :)

Oh? I did not know of this 👀 What was the motivation?

[zhujun98]

So, in general I'm not a fan of forking libraries for internal use; firstly because it puts the burden of maintenance (however small) on us, secondly because I don't think it's in the FOSS spirit to not contribute our improvements back to the community. As much effort as I'm sure it took to optimize and clean up pyqtgraph, I would rather those improvements go upstream :)

You will have to break a lot of legacy code from others, which is basically mission impossible. pyqtgraph trades flexibility for speed because of the broad user community. The job of DA however is to facilitate the experiments at XFEL at much as possible. And this does not prevent you from contributing back to the community, such as what I did also for xtensor etc. I also look forward to your contribution to the community.

I did make a critical contribution to pyqtgraph
pyqtgraph/pyqtgraph#1057 (comment)
to improve the line plot speed, otherwise they would be even slower.

Oh? I did not know of this 👀 What was the motivation?

Because we all know there are a lot of craps in pyqtgraph.

Again, uses would be happy to see more new features and improvements in the future instead of the other way round.

[leguyader]

To be honest, I don't agree here. You may have seen a few use cases in the hutch, but maybe not all of them. EXtra-foam is oftentimes used in multiple big screens together. If you add up the slowdown of multiple plots, it makes a huge difference.

I can confirm also that at SCS we usually have extra-foam showing many plots and the performance quickly degrade as we show more plots. Slower plotting is probably not the way to go.

[zhujun98]

Image	1024x1280	33	40

pyqtgraph uses the grey colormap by default, which is faster than the other colormap because of some shortcut in rendering. This may be the reason of the performance difference. Anyway, I come to announce that foamgraph is already in a good shape). I finally found some time to replace the pyqtgraph "engine" and it is almost done :-) Of course, a lot of new features have been added.

I'd like to draw your attention and, if you had time, please give some feedback @JamesWrigley @ebadkamil @CammilleCC @dgoeries. Another sprint is still required to make it a stable version. For now, I'd like to know which critical plotting features (e.g. plot type, context menu functions) are missing in the old version. Of course, I also have some "crazy" ideas in my mind and foamgraph may have another evolution soon.

Performance-vise, it should be even faster because the original ViewBox is replaced with Canvas.

[jo-moeller]

Are image plots with logarithmic colorbar possible?

[zhujun98]

Are image plots with logarithmic colorbar possible?

@jo-moeller Yes. In foamgraph one sets log scale via the context menu of each axis instead of the view box. So by checking the log scale on the axis of the colorbar, the color scale is expected to become logarithmic. But the display of the colorbar itself does not change. I still need to implement it zhujun98/foamgraph#67. But it is not complicated. I was actually wondering why one would like to make the axis of the colorbar logarithmic, so there is a good use case. Thank you!

[j9ac9k]

greetings from pyqtgraph land. Just chiming in here to see if we can help here. Since this PR was opened, we've had significant performance improvements. Some of the recent points we've hit regarding performance improvements. I'm not sure what version was checked at the start of this PR, I will assume it was 0.12.3 since that was the current version released at the time this PR was created.

Some notable changes

• (master branch, will be in the next release) - PlotDataItem.setDownsample() sees a significant performance improvement.
• 0.13+ toggle enableExperimental and we see a 5-15% performance improvement (varies by platform) on curves
  • We do some horrible things to construct QPainterPath w/o serialization/deserialization see here
• 0.12.4 We introduced high-performance line plots for thick lines (lines thicker than 1 pixel were a major pain point for some time).

0.12.3 was released at the time this PR was created, but I feel like I should mention that in this version, there was a significant performance improvement made to line plots. In 0.12.2 a performance improvement was made in our scatter plots, but it largely hasn't seen much update since. In that same version, we also added optional numba support for some image item related performance...but it's impossible to say if that would be something you would benefit from w/o knowing more.

Anyway, if upstream is still lagging in some way, I'd be happy to take a closer look.

EDIT: if you all have 3x performance improvement on your internal variants of ScatterPlots vs. us; I want to see what you all are doing 😆 (curious about the other plot types too) ... if we wouldn't break functionality, we would certainly love to integrate any performance enhancements upstream.