-
Notifications
You must be signed in to change notification settings - Fork 80
Draft: Add 'dynamic colormap' mode #4271
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
base: main
Are you sure you want to change the base?
Conversation
|
|
It would be more relevant to adjust the colormap over the ROI of screen pixels (rather than image pixels). |
Shall I try to implement? Actually, I am not entirely convinced by this. If zoomed in, one image pixel might be represented by a 20x20 square of screen pixels. Adjusting over this does not make sense... |
|
For some reason, it seems that no check is done on the PR. There were some yesterday (and they failed, for reason that are beyond my understanding...). No more today. |
|
@t20100 : I asked the review to you, because Henri opened the PR... We discussed with Henri: I ask a review as a first iteration. |
IMO it can be done in another PR. Let's try to have the good structure and something fairly working first. Then it can be modified later. |
😆 I can do the pre-review |
Indeed this is a bit weird. |
| # dynamic colormap | ||
| self._dynamicColormap = qt.QPushButton(self) | ||
| self._dynamicColormap.setCheckable(True) | ||
| self._dynamicColormap.setEnabled(False) | ||
| self._dynamicColormap.setText("Dynamic colormap") | ||
| self._dynamicColormap.setIcon(icons.getQIcon("add-shape-rectangle")) | ||
| self._dynamicColormap.setCheckable(True) | ||
| # self._dynamicColormap.toggled.connect( | ||
| # self._handleDynamicColormap, type=qt.Qt.QueuedConnection | ||
| # ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this is unused at the moment. It should be either removed or fully integrated on the ColormapDialog. As a reminder the ColormapDialog is this widget:
IMO better to remove this code first and have the processing working as a PlotAction
Note: in case you want to integrate it 'setCheckable` seems repeated.
'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I remove it completely since I see no point in having a checkbox in this dialog which does nothing but what the icon in the widget toolbar does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think removing is the best. But indeed those are covering two 'different' uses cases / concept.
- The QAction
EachPlotWindow(likePlot2D) can define a set of action tunable by the users. By default not all are here (and yours might not be part of the default one - to be discussed).
But anyway everyone definingPlotWindowcan define which action are present on the toolbar or not. - The ColorbarDialog
Well this widget is common to everyone and everyone will access to all the options for now. IMO at the end we should find your option. But again it can come in a second step. As we might also want to define the shape; the radius...
src/silx/gui/plot/PlotInteraction.py
Outdated
| ROI_SIZE = (10, 10) # (y,x). The ROI <<radius>> | ||
|
|
||
| @staticmethod | ||
| def compute_vmin_vmax(data, dataPos): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider moving this API from public to protected API ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. By this, you simply mean renaming it _compute_vmin_vmax() right?
| # Set new min and max | ||
| colormap.setVRange(vmin, vmax) | ||
| item.setColormap(colormap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misslead you sorry, indeed the update will be applied even if 'item.setColormap(colormap)' is not called. I had the old object in mind my bad 🙏
| # Set new min and max | |
| colormap.setVRange(vmin, vmax) | |
| item.setColormap(colormap) | |
| # Set new min and max | |
| colormap.setVRange(vmin, vmax) |
| elif key == qt.Qt.Key_W: | ||
| self.setInteractiveMode("dynamic_colormap") | ||
| elif key == qt.Qt.Key_P: | ||
| self.setInteractiveMode("pan") | ||
| elif key == qt.Qt.Key_Z: | ||
| self.setInteractiveMode("zoom") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keyboard shortcut could be added to the PR description. It could be useful to get them back quickly... and to discuss them.
On this PR I would only propose a shortcut for the 'dynamic_colormap'. Others can be propose on dedicated PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we want to add this to PlotWidget, at least without a review of all keyboard shortcuts + it's easy to add in an application
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the point, but it's really a key feature to me: to make it light... : in that respect, adding the icon in the colormap widget would not make much sense IMO. But maybe it is independent where the icon is and how to activate the interaction mode...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe it is independent where the icon is and how to activate the interaction mode...
Yes!
it's really a key feature to me
Sure, but this doesn't mean it is a key feature for other usage of PlotWidget (e.g., when plotting curves). That's why I propose to start by having this shortcut in your application rather than as a default in PlotWidget
| class TestSelectDynamicColormap(): | ||
|
|
||
| def test_evaluate_roi(self): | ||
| """Test evaluate_roi() method""" | ||
| plot = PlotWidget() | ||
| plot.setInteractiveMode("dynamic_colormap", shape="rectangle", label="test") | ||
|
|
||
| interaction = plot.getInteractiveMode() | ||
| assert interaction["mode"] == "dynamic_colormap" | ||
| assert isinstance(plot.interaction()._eventHandler, DynamicColormapMode) | ||
|
|
||
| # Test with a rectangle | ||
| roi = numpy.arange(484).reshape((22,22)) | ||
| vmin, vmax, bb_x, bb_y = DynamicColormapMode.compute_vmin_vmax(roi,(11,11)) | ||
| assert vmin == 23 and vmax == 460 and bb_x == (1,1,21,21) and bb_y == (1,21,21,1) | ||
|
|
||
| roi = numpy.arange(484).reshape((22,22)) | ||
| vmin, vmax, bb_x, bb_y = DynamicColormapMode.compute_vmin_vmax(roi,(0,0)) | ||
| assert vmin == 0 and vmax == 207 and bb_x == (0,0,10,10) and bb_y == (0,10,10,0) | ||
|
|
||
| roi = numpy.arange(484).reshape((22,22)) | ||
| vmin, vmax, bb_x, bb_y = DynamicColormapMode.compute_vmin_vmax(roi,(21,21)) | ||
| assert vmin == 253 and vmax == 483 and bb_x == (11,11,22,22) and bb_y == (11,22,22,11) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new test must be done with pytest instead of unittest.
And maybe it could be split into two tests instead of one.
So more something like
def test_dynamic_colormap_interaction(qapp):
plot = PlotWidget()
plot.setInteractiveMode("dynamic_colormap", shape="rectangle", label="test")
interaction = plot.getInteractiveMode()
assert interaction["mode"] == "dynamic_colormap"
assert isinstance(plot.interaction()._eventHandler, DynamicColormapMode)
def test_dynamic_colormap_vmin_vmax_calculation(qapp):
roi = numpy.arange(484).reshape((22,22))
vmin, vmax, bb_x, bb_y = DynamicColormapMode.compute_vmin_vmax(roi,(11,11))
assert vmin == 23 and vmax == 460 and bb_x == (1,1,21,21) and bb_y == (1,21,21,1)
roi = numpy.arange(484).reshape((22,22))
vmin, vmax, bb_x, bb_y = DynamicColormapMode.compute_vmin_vmax(roi,(0,0))
assert vmin == 0 and vmax == 207 and bb_x == (0,0,10,10) and bb_y == (0,10,10,0)
roi = numpy.arange(484).reshape((22,22))
vmin, vmax, bb_x, bb_y = DynamicColormapMode.compute_vmin_vmax(roi,(21,21))
assert vmin == 253 and vmax == 483 and bb_x == (11,11,22,22) and bb_y == (11,22,22,11)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the comment on unittest vs. pytest. OK for the split. DOne.
Co-authored-by: payno <[email protected]>
Co-authored-by: payno <[email protected]>
Co-authored-by: payno <[email protected]>
Co-authored-by: payno <[email protected]>
You could have a mix of both: use a 20x20 square on screen and then make sure it is at least 20x20 pixels in the image |
t20100
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a structural issue of where to implement this feature: IMO it should be implemented "on-top" of PlotWidget rather than in PlotInteraction.py.
There's also a question of where this feature should be provided: ColormapDialog, ImageToolBar, a QAction that is not included in plots by default ? I would suggest to give a try to the colormap dialog since it is related to it.. even if there's quite a lot of things already.
Finally the icon does not convey the meaning of a dynamic colormap IMO.
Use https://github.com/silx-kit/silx/blob/main/src/silx/resources/gui/icons/image-select-box.png ?
| def main(): | ||
| app = qt.QApplication([]) | ||
|
|
||
| # Create the ad hoc plot widget and change its default colormap | ||
| x = numpy.zeros((100, 100),dtype=numpy.float32) | ||
| x[:50,:50] = numpy.random.randn(50,50) | ||
| x[:50,50:] = 10 * numpy.random.randn(50,50) | ||
| x[50:,:50] = 100 * numpy.random.randn(50,50) | ||
| x[50:,50:] = 5 * numpy.random.randn(50,50) | ||
|
|
||
| example = Plot2D() | ||
| example.addImage(x) | ||
| example.show() | ||
|
|
||
| app.exec() | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is nothing specific about dynamic colormap in this code sample, I would remove it.
| return super().endDrag(startPos, endPos, btn) | ||
|
|
||
|
|
||
| class DynamicColormapMode(ItemsInteraction): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PlotInteraction.py is not the place for "specific" interaction (i.e., that depends on specific items), and it's best to not add features in this module since it would benefit from a large refactoring.
I'll look for an alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I understand that this interaction only aims 2d plots. Then, its place could be in the ImageToolbar. In the same time, it is really an interactive mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is an interaction mode so independent of the ImageToolbar.
I'll look at it an propose a way to structure this
| else: | ||
| item = result.getItem() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| else: | |
| item = result.getItem() | |
| item = result.getItem() |
| def _compute_vmin_vmax(data: numpy.ndarray, dataPos: tuple[float, float]): | ||
| """Compute the min and max values of the data in a ROI centered on (x,y)""" | ||
| roi_size = DynamicColormapMode.ROI_SIZE | ||
| idx_x, idx_y = int(dataPos[0]), int(dataPos[1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't take care of origin and scale of the Image item
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. But it is not incompatible. In the worst case, it does not improve the local contrast. The user can choose either a specific color scale or to use that feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The image can have an offset from origin and a scaling so the coordinates of the plot dataPos cannot be converted directly to indices in the image array.
As it is, the local contrast will be computed on a wrong area of the image if this image has an offset or scale.
This should be handled and it should not be an issue to do so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You retrieve the data from the an item upper (on the stack).
I think this data is an instance of ImageData. If I m right you should have all your need there.
| # sigDynamicColormapModeChanged = qt.Signal(object) | ||
| # """ | ||
| # Signal emitted when the dynamic colormap changed | ||
|
|
||
| # It provides the source as passed to :meth:`setInteractiveMode`. | ||
| # """ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # sigDynamicColormapModeChanged = qt.Signal(object) | |
| # """ | |
| # Signal emitted when the dynamic colormap changed | |
| # It provides the source as passed to :meth:`setInteractiveMode`. | |
| # """ |
| elif key == qt.Qt.Key_W: | ||
| self.setInteractiveMode("dynamic_colormap") | ||
| elif key == qt.Qt.Key_P: | ||
| self.setInteractiveMode("pan") | ||
| elif key == qt.Qt.Key_Z: | ||
| self.setInteractiveMode("zoom") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we want to add this to PlotWidget, at least without a review of all keyboard shortcuts + it's easy to add in an application
| """QAction controlling the colormap mode of a :class:`.PlotWidget`. | ||
| This mode adjusts the colormap based on a small region around the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| """QAction controlling the colormap mode of a :class:`.PlotWidget`. | |
| This mode adjusts the colormap based on a small region around the | |
| """QAction controlling the colormap mode of a :class:`.PlotWidget`. | |
| This mode adjusts the colormap based on a small region around the |
| self._dynamicColormapAction = actions.mode.DynamicColormapAction( | ||
| parent=self, plot=plot | ||
| ) | ||
| self.addAction(self._dynamicColormapAction) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will add it to all kind of plots (curves, scatters) while it only works with images, so it's not the place.
This could be added to the colormap dialog, though it is already quite crowded or the ImageToolBar... or not included in any toolbar and added by the application that wants it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark as before: could be in ImageToolbar but is truly an interactive mode. On the top of that, this ImageToolbar does not seem to be used on the PlotWindow/ImageView that displays images. I can add the action in the Plot2D toolbar. But don't really know how to handle the interaction mode (hence the initial positioning in the PlotWidget/InteractionToolbar.
| elif isinstance(self._eventHandler, DynamicColormapMode): | ||
| return {"mode": "dynamic_colormap"} | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid to add a "generic" interaction for a specific case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I get what is behind the 'generic' interaction. Do you suggest that we have some kind of interaction subset / categories ?
Because at the moment we need a specific interaction mode to avoid conflict with other modes.
|
Here is a minimal working prototype of how to implement it without changing the internals of from typing import Any
import numpy as np
from silx.math.combo import min_max
from silx.gui import qt
from silx.gui import icons
from silx.gui.plot import items
from silx.gui.plot.items.core import PickingResult
from silx.gui.plot.PlotWidget import PlotWidget
from silx.gui.plot.actions import PlotAction
from silx.gui.plot import Plot2D
class DynamicColormapAction(PlotAction):
def __init__(self, plot: PlotWidget, parent: qt.QWidget | None = None):
super().__init__(
plot,
icon=icons.getQIcon("image-select-box"),
text="Dynamic colormap mode",
tooltip="Update the colormap according to the mouse position in the plot",
triggered=self.__triggered,
checkable=True,
parent=parent,
)
def __triggered(self, checked: bool) -> None:
plotWidget = self.plot
if checked:
plotWidget.sigPlotSignal.connect(self.__plotChanged)
else:
plotWidget.sigPlotSignal.disconnect(self.__plotChanged)
def __pickTopMostImageData(self, x: float, y: float) -> PickingResult | None:
for pickingResult in self.plot.pickItems(
x,
y,
condition=lambda item: isinstance(item, items.ImageDataBase)
):
return pickingResult
return None
def __plotChanged(self, event: dict[str, Any]) -> None:
if event.get("event") != "mouseClicked":
return
pickingResult = self.__pickTopMostImageData(event["xpixel"], event["ypixel"])
if pickingResult is None:
return
image = pickingResult.getItem()
indices = pickingResult.getIndices()
if indices is None:
return
row, col = indices[0][0], indices[1][0]
# TODO define ROI in pixel coords and compute range accordingly
value = image.getData(copy=False)[row, col]
vmin, vmax = value - 10000, value + 10000
colormap = image.getColormap()
colormap.setVRange(vmin, vmax)
app = qt.QApplication([])
plot = Plot2D()
dynamicColormapAction = DynamicColormapAction(plot)
plot.toolBar().insertAction(plot.getColormapAction(), dynamicColormapAction)
plot.addImage(np.arange(1024.**2).reshape(1024, 1024))
plot.show()
app.exec()It's missing:
|
|
BTW, here are some alternative for the implementation:
|


close #4263