Implemented pColorMeshItem#1273
Conversation
|
Updating the base to merge to master; I need to update |
|
Ooof...this CI is blowing up due to matplotlib not being installed... looking at the diff, matplotlib is wanted for the An approximation of viridis is available in https://github.com/bokeh/bokeh/blob/7cc500601cdb688c4b6b2153704097f3345dd91c/bokeh/palettes.py I understand that this is outside the scope of this PR, but having matplotlib as a test dependency is rough, especially for just those methods. |
|
Ideally we'd have test coverage of the matplotlib exporter/widget, so it could be required for tests eventually anyway. That said, I agree it would be better to avoid requiring it for users for the color map. Also the class name might be more consistent with the library as PseudoColorMeshItem or PColorMeshItem (latter might be more intuitive/recognizable). |
|
For this PR, I did not have time for a reasonable review, just from first glance I noticed that the naming For matplotlib colormaps, I've found this repository by @honkomonk some time ago: https://github.com/honkomonk/pyqtgraph_sandbox It might not be the ideal way though, but maybe a good start if someone wants to look into that. |
|
Regarding the colormaps (so a bit off-topic): The longer answer: Bokeh just copied matplotlibs color tables and converted them into their format. So no magic is happening there. PS: It's an interesting story how Viridis was created: https://bids.github.io/colormap/ |
|
Hi all, I removed all dependencies to matplotlib so that we can focus on the Item itself. |
|
all pipelines failed with: EDIT: I think you need to rename the file:
|
|
This is weird, I renamed all the files with an uppercase "P": And the example file works on my laptop and gives this: Is there a way for me to test these "pipeline" online before pushing anything? |
|
you could try running I would recommend renaming the file to lower case in windows, then run
|
|
Ok, will try the pytest thing. |
|
FYI that looks to have done the trick, some pipelines have passed (some are still running). EDIT: CI came back all green! |
|
Just out of curiosity: Is the |
The idea was to copy the matplotlib function @honkomonk @j9ac9k What will be the next step for me? I would like to have a final item with axis and histogram but would like your advice on the road to take to go there. |
| """ | ||
| **Bases:** :class:`GraphicsObject <pyqtgraph.GraphicsObject>` | ||
|
|
||
| TODO |
|
|
||
| self.axisOrder = getConfigOption('imageAxisOrder') | ||
|
|
||
| if cmap in list(Gradients.keys()): |
There was a problem hiding this comment.
Do you need to convert to a list here? doing in lookups in the dict.keys() I know works in python3, does it not work in python2? (added bonus of not converting to list, is that in lookups in the hashed objects are significantly faster.
|
|
||
| if self.z is None: | ||
| return QtCore.QRectF(0., 0., 0., 0.) | ||
| return QtCore.QRectF(0., 0., float(self.width()), float(self.height())) No newline at end of file |
|
@edumur this PR looks good to me, I made some in-line comments for suggested changes and questions. I haven't worked w/ these graphical objects before so I'm going to let some other folks around here give things a look over. Thanks for the PR! |
|
@j9ac9k ok, I will work on your comments and follow with a PColorMeshView that will offer axis and histogram as ImageView. |
Make an example displaying more clearly the Item capability. Correct few bugs in the Item class. Improve overall comments.
Allow user to set the polygons edge color.
|
ok so what I did is putting myself in the examples folder and then did the git rename command. |
|
closer, all pyside pipelines are failing I can replicate locally with a PySide Qt bindings installed and running (so you don't have to run the entire test suite). In case it helps, here's the captured output from pytest when I ran locally |
|
Doing a little work procrastination; tinkering with this, wrapping the qpoint's with a will keep messing with it... |
|
Got it working with both configurations made the change in |
|
I am thankful ! |
|
I'm good with this as is, @ixjlyons any other requested changes? |
ixjlyons
left a comment
There was a problem hiding this comment.
This is looking pretty good to me. There are some issues with docs noted with line comments (sidenote: maybe we should have a docs build job just to catch warnings and errors before merging).
Also, I don't know if anyone else is observing this, but the example shows some artifacts at the edges of the mesh when I zoom and pan around. I'll try to put up a screenshot later today.
| GridItem | ||
| HistogramLUTItem | ||
| ImageItem | ||
| PColorMeshItem |
There was a problem hiding this comment.
I'm not actually sure these "make" files do anything in the current docs build. I'm not aware of something like this being part of sphinx - my guess is this was part of an older build setup to generate the stub files for the API reference. Could you copy one of the other files in the doc/source/graphicsItems folder?
There was a problem hiding this comment.
I do not understand what you mean by "Could you copy one of the other files in the doc/source/graphicsItems folder?".
There was a problem hiding this comment.
For now at least, the API docs are manually created, so the .rst files in doc/source/graphicsItems/ are all stub files with pretty much the same content. So the idea was to copy one of those files and replace the class name with PColorMeshItem. legenditem.rst would be a good example since it includes the top class-level doc and the __init__ method.
There was a problem hiding this comment.
Done, the file looks like this:
PColorMeshItem
==============
.. autoclass:: pyqtgraph.PColorMeshItem
:members:
.. automethod:: pyqtgraph.PColorMeshItem.__init__
| If x and y is None, the polygons will be displaced on a grid | ||
| otherwise x and y will be used as polygons vertices coordinates as: | ||
|
|
||
| (x[i+1, j], y[i+1, j]) (x[i+1, j+1], y[i+1, j+1]) |
There was a problem hiding this comment.
These diagrams probably need to be put in a code environment to render correctly.
There was a problem hiding this comment.
You can take a look at how matplotlib is doing it: https://matplotlib.org/3.3.0/_modules/matplotlib/axes/_axes.html#Axes.pcolormesh
Basically, you can make a double-colon :: where you already have one and then indent the code block one level more than the text it's surrounded by.
There was a problem hiding this comment.
ok, it looks like this:
"""
otherwise x and y will be used as polygons vertices coordinates as::
(x[i+1, j], y[i+1, j]) (x[i+1, j+1], y[i+1, j+1])
+---------+
| z[i, j] |
+---------+
(x[i, j], y[i, j]) (x[i, j+1], y[i, j+1])
"""|
Yep, I see the artifacts as well: I tried to remove them but so far it didn't work. |
| return | ||
|
|
||
| profile('p.drawPicture') | ||
| p.drawPicture(0, 0, self.qpicture) |
There was a problem hiding this comment.
you may be able to redo this as something like
if self.qpicture is not None:
self.qpicture.play(p)The implementation taken from TargetItem.py https://github.com/pyqtgraph/pyqtgraph/blob/master/pyqtgraph/graphicsItems/TargetItem.py#L73-L76
There was a problem hiding this comment.
wait, that probably does nothing, was reading the docs:
This function does exactly the same as QPainter::drawPicture() with (x, y) = (0, 0).
There was a problem hiding this comment.
Ok, so this is clearly it because if I add a coefficient in the boundingRect method to return, for instance, QtCore.QRectF(0., 0., float(self.width())*2., float(self.height())*2.) then the artifacts disappear.
I tried fix to make it better but so far it didn't work well...
|
|
||
| if shapeChanged: | ||
| self.informViewBoundsChanged() | ||
|
|
There was a problem hiding this comment.
If this would change the result of boundingRect(), then you should also call self.prepareGeometryChange().
See: https://doc.qt.io/qt-5/qgraphicsitem.html#boundingRect
|
|
||
|
|
||
|
|
||
| def boundingRect(self): |
There was a problem hiding this comment.
Suggestion: returning QtCore.QRectF(self.picture.boundingRect()) here might help with the motion artifacts.
(See BarGraphItem for example)
|
Thanks all, I implemented your suggestions. |
|
hey @edumur just wanted to let you know I haven't forgotten about this, I want to get this merged as this is an awesome feature. I just started experimenting w/ |
|
I'm likely not telling you anything you don't know; but doing some debugging here, and I suspect the issue is that your I added a couple of print statements if shapeChanged:
self.informViewBoundsChanged()
self.prepareGeometryChange()
print("shape changed")and def paint(self, p, *args):
profile = debug.Profiler()
if self.z is None:
return
profile('p.drawPicture')
p.drawPicture(0, 0, self.qpicture)
print(self.boundingRect())wanting the console I get the following So we're having boundingRect change, but not calling EDIT: so storing the currentBoundingRect = self.boundingRect()
if currentBoundingRect != self.previousBoundingRect:
self.previousBoundingRect = currentBoundingRect
self.prepareGeometryChange()
self.informViewBoundsChanged()
print("shape changed") |
|
Ha, I think I got it, sure enough, a pretty dead simple change; move |
|
|
||
| # We set the pen of all polygons once | ||
| if self.edgecolors is None: | ||
| p.setPen(QtGui.QColor(0, 0, 0, 0)) |
There was a problem hiding this comment.
Probably need to do p.setPen(fn.mkPen(QtGui.QColor(0, 0, 0, 0))) as I think setPen requires a QPen object
| if np.any(self.x != args[0]) or np.any(self.y != args[1]): | ||
| shapeChanged = True | ||
|
|
||
| profile = debug.Profiler() |
There was a problem hiding this comment.
Useful for debugging, but might as well remove from the version we merge.
| for yi in range(self.z.shape[1]): | ||
|
|
||
| # Set the color of the polygon first | ||
| # print(xi, yi, norm[xi][yi]) |
There was a problem hiding this comment.
no need for commented out print statement
| # Set the color of the polygon first | ||
| # print(xi, yi, norm[xi][yi]) | ||
| c = lut[norm[xi][yi]] | ||
| p.setBrush(QtGui.QColor(c[0], c[1], c[2])) |
There was a problem hiding this comment.
probably should also wrap this with a fn.mkBrush()
examples/PColorMeshItem.py
Outdated
| new_z) | ||
|
|
||
| i += wave_speed | ||
| QtCore.QTimer.singleShot(1000/fps, updateData) |
There was a problem hiding this comment.
I'm getting a deprecation warning here for using float
/Users/ogi/Developer/pyqtgraph/examples/PColorMeshItem.py:75: DeprecationWarning: an integer is required (got type float). Implicit conversion to integers using __int__ is deprecated, and may be removed in a future version of Python.
QtCore.QTimer.singleShot(1000/fps, updateData)
Probably should change this line to:
QtCore.QTimer.singleShot(1000//fps, updateData)|
|
||
| self.qpicture = QtGui.QPicture() | ||
| p = QtGui.QPainter(self.qpicture) | ||
|
|
There was a problem hiding this comment.
totally optional here, but I've been experimenting on my own, but consider setting the anti-aliassing render hint
p.setRenderHint(QtGui.QPainter.Antialiasing)With this set, the lines were actually visible on my screen; admittedly that may not be what you want.
Add a parameter "antialiasing". Remove profiler Add pyqtgraph mkPen
|
Hi @j9ac9k, Thanks for the comments. |
|
That's absolutely perfect; thanks so much for this feature @edumur , merging! |
As discussed in the issue #1262 I proposed here a minimal implementation of an item mimicking the
pcolormeshfunction of matplotlib.As I didn't know what to do to make the pull request valid I decided to make it as minimal as possible.
I am willing to integrate it more into pyqtgraph if someone gives me some directions.
In particular a I think a kind of pColorMeshWidget with axis and Histogram would be an excellent end result of this work.
Fixes #146
Fixes #1262