Conversation
| "ASCII from: <https://matplotlib.org/3.2.1/api/_as_gen/ | ||
| matplotlib.pyplot.pcolormesh.html>". | ||
| cmap : str, default 'viridis | ||
| cmap : pg.ColorMap, default 'viridis' |
There was a problem hiding this comment.
this type isn't quite correct, this should be pg.ColorMap or str no?
There was a problem hiding this comment.
So the idea was to "deprecate" use of str to specify the colormap and to make the user pass in ColorMap objects only, where the deprecation is done by simply not advertising that str could be passed in.
Alternatively, we could forgo the use of GradientEditorItem entirely and let str refer to a ColorMap name.
After all, the original PColorMeshItem was created before there was a easily usable ColorMap.
There was a problem hiding this comment.
As stated in the other comment, I'm good w/ ditching GradientEditorItem, might be a bit out of scope for this PR.
I'm good w/ deprecating the use of str and just using a known pg.ColorMap object.
| # but this will only use colormaps from Gradients. | ||
| # Note that the colors will be wrong for the hsv colormaps. | ||
| if isinstance(self.cmap, str): | ||
| if self.cmap in Gradients: |
There was a problem hiding this comment.
Sort of hate that we're limiting ourselves to these when referencing by string instead of using the functionality in colormap.py but I suppose that's for another PR.
There was a problem hiding this comment.
"thermal", "flame", "yellowy", "bipolar" are the colormaps available in GradientEditorItem but not in colors/maps.
Can we simply check for those 4 to fetch from GradientEditorItem and the rest from ColorMap?
Alternatively, PColorMeshItem being fairly new as it were, can we just ditch the use of GradientEditorItem?
Personally, I don't have an issue with having to explicitly instantiate a pg.ColorMap instead of having the convenience of passing in a string.
There was a problem hiding this comment.
Nice catch on the list of colors there that are not available in colors/maps
I'm 100% onboard with ditching GradientEditorItem, but I feel like this might be a bit out of scope for this PR, especially if we need to move some colors into colors/maps in order to retain equivalent capabilities.
There was a problem hiding this comment.
I need to look at this a little bit, but we could assume for now that by the time this is ready to merge, colormap.get('thermal') will not error out and return something we consider acceptable. Would that simplify things?
There are some options for how to set that up; At least 'thermal' and 'flame' map pretty well to some of the existing maps; If we want to preserve full pixel-by-pixel accuracy, we could also copy over the definitions; they are really short for the maps in question.
I would prefer not to add these maps to the data files and documentation, though: They have the usual problems of alternating region with sharp contrast (the purple-yellow step in "yellowy") with long stretches where there is no easily visible change (everything else in "yellowy"). But we can get rid of the link to GradientEditorItem while keeping old code working - and without too much trouble.
There was a problem hiding this comment.
But I am also good with having to write out
PColorMeshItem( colorMap = pg.colormap.get('viridis') ) instead of the string shorthand. It keeps the API simpler (once the legacy string gets deprecated). And this is also what ColorBarItem does now, so it might be a good idea for consistency.
https://pyqtgraph.readthedocs.io/en/latest/graphicsItems/colorbaritem.html#pyqtgraph.ColorBarItem
Can I bring up the option of making the officially advertised parameter colorMap instead of the abbreviated cmap? That's what we have in ColorBarItem now, where we deprecated cmap with a target of summer 2022:
Since this is using **kwargs already, maybe checking for both variants is not too painful.
We could also keep cmap as the hidden legacy parameter that accepts the old strings, while the colorMap keyword wants a ColorMap. But it isn't clear to me if that is simpler/better. :)
There was a problem hiding this comment.
examples/NonUniformImage.py inserts a custom color map into GradientEditorItem. Let's hope people don't do that in the wild...
|
I'm good with this PR as is @pijyoi only question is if we should try and remove the use of If we did that change, we should see about migrating the colors in Sure wish the source in |
Ok, I will leave the backwards-compatible use of GradientEditorItem as is.
The ImageView widget is currently limited to the colormaps in GradientEditorItem. But the small number of colormaps also has the advantage of allowing it to show to the user without a discernable lag a mini-view of how the colormap looks like. |
This could be cut down to 2 by pre-creating 256 |
| c = lut[norm[xi][yi]] | ||
| p.setBrush(fn.mkBrush(QtGui.QColor(c[0], c[1], c[2]))) | ||
| scale = len(lut) - 1 | ||
| rng = self.z.ptp() |
There was a problem hiding this comment.
This is probably excessive optimization, but I previously read (e.g. here: https://stackoverflow.com/questions/12200580/numpy-function-for-simultaneous-max-and-min )
that numpy's ptp() function internally just calls "min" and "max" on the data.
If that is true, we are now effectively making three passes over self.z, and are specifically running z.min() twice (since we do that again in L233). Then
z_min = self.z.min()
z_max = self.z.max()
rng = z_max - z_minmight be faster since we can reuse z_min below.
I guess that has zero effect on actual execution time, though.
There was a problem hiding this comment.
Right, no need to optimize excessively, but also no need to pessimize the code unnecessarily either.
|
Reducing the number of method calls from 3 to 2 within the inner-most loop yielded a speedup of 3/2. (Please use squash when merging) |
|
LGTM, thanks for this @pijyoi ! |
Generally, a ~4x speedup was obtained on the example.
The remaining bottleneck is that 3 method calls are still required per polygon.
Even if Qt had a batch
drawConvexPolygons, it wouldn't help; because each polygon potentially has a different color.It might be possible to iterate on the outer loop by color, but that would require treating each polygon as having unique vertices, rather than being in a regular grid.
Changes: