Add widgets for setting histogram bins #242

p-j-smith · 2024-01-11T11:48:03Z

Fixes #239

added QDoubleSpinBox widgets for setting the start and stop edges of the histogram bins (default to the min and max data values, same as existing values used)
added a QSpinBox widget for setting the number of bins to use (defaults to 100, same as the existing value used)
add callbacks to re-draw figure when value of widgets change
set the dtype for np.linspace when creating bins (so integer bin limits are used for integer images). This caused the test_histogram_2D test to fail (as the test layer has integer data but float bins were being used) so I've updated the baseline image - this is now done in Use integer bins for integer data in HistogramWidget #244
added a test for the HistogramWidget when the bin parameters are set from the widgets
add start, stop, and num_bins parameters to _get_bins function so that bin parameters set in the widgets can be used for settings bins
show notification if the bin parameters requested by the user have been modified before plotting (this can happen with integer data as we need to make sure the bins have integer widths)

napari-mpl-hist-bins.mov

dstansby · 2024-01-11T11:57:18Z

Thanks for the PR! Could you put the integer dtype histogram change into a separate PR to make it easier to review?

p-j-smith · 2024-01-11T12:53:56Z

Could you put the integer dtype histogram change into a separate PR to make it easier to review?

Yep, will do 🙂

p-j-smith · 2024-01-11T13:17:23Z

src/napari_matplotlib/histogram.py

+        bins_start = QDoubleSpinBox()
+        bins_start.setObjectName("bins start")
+        bins_start.setStepType(QAbstractSpinBox.AdaptiveDecimalStepType)
+        bins_start.setRange(-1e10, 1e10)


I wasn't sure what a reasonable range would be (rather than the default of 0 to 100)?

p-j-smith · 2024-01-11T13:20:47Z

src/napari_matplotlib/histogram.py

+        # Only allow integer bins for integer data
+        # And only allow values greater than 0 for unsigned integers
+        n_decimals = 0 if np.issubdtype(layer_data.dtype, np.integer) else 2
+        is_unsigned = layer_data.dtype.kind == "u"
+        minimum_value = 0 if is_unsigned else -1e10
+
+        bins_start = self.findChild(QDoubleSpinBox, name="bins start")
+        bins_stop = self.findChild(QDoubleSpinBox, name="bins stop")
+        bins_start.setDecimals(n_decimals)
+        bins_stop.setDecimals(n_decimals)
+        bins_start.setMinimum(minimum_value)
+        bins_stop.setMinimum(minimum_value)


it's necessary to set the minimum bin values to 0 for unsigned integers - if negative bin limits are selected for data with unsigned integers, plt.hist complains that bins must be monotonically increasing (as there's an integer underflow in the bins produced bynp.linspace)

it's not necessary to set the decimals to 0 for integers, but makes it clear to the users that the data are integers and float bin limits are not appropriate

Also set np.linspace dtype based on image dtype

n_bins corresponds to number of bin edges rather than bins

…her than number of bin edges

p-j-smith · 2024-01-15T17:13:12Z

src/napari_matplotlib/histogram.py

+        # Disable callbacks whilst widget values might change
+        for widget in self._bin_widgets.values():
+            widget.blockSignals(True)
+
+        self._bin_widgets["start"].setDecimals(n_decimals)
+        self._bin_widgets["stop"].setDecimals(n_decimals)
+        self._bin_widgets["start"].setMinimum(minimum_value)
+        self._bin_widgets["stop"].setMinimum(minimum_value)
+
+        for widget in self._bin_widgets.values():
+            widget.blockSignals(False)


temporarily disable the callbacks otherwise the plot is re-drawn each time the widget value changes

src/napari_matplotlib/histogram.py

p-j-smith · 2024-01-15T17:16:00Z

src/napari_matplotlib/histogram.py

+        if data.dtype.kind in {"i", "u"}:
+            # Make sure integer data types have integer sized bins
+            step = abs(self.bins_stop - self.bins_start) // (self.bins_num)
+            step = max(1, step)
+            bins = np.arange(self.bins_start, self.bins_stop + step, step)


I think it would be good to show a notification when the actual number of bins used differs from that requested in the widgets, but perhaps this could be a different PR?

I've added the notification now, but happy to remove it and add separately if it's easier

dstansby · 2024-05-25T08:40:31Z

I am finally thinking about this (sorry it took so long!), and my current thoughts are to keep things simple we should just have a setting for the number of bins, and show the full range of data without being able to control the start/stop. I am open to adding confiurable start/stop in the future if there's a compelling use case, but for now I think configuring the number of bins is a nice feature that isn't too complicated by itself.

Would you be able to update this PR (or open a new one) to just have the number of bins configurable? Obviously no rush at all 😄, but let me know if it isn't likely to be soon in case I get the bug to implement it

p-j-smith · 2024-05-25T10:10:48Z

Yeah good idea, it became surprisingly complicated! I've removed the notification of requested vs actual bins that I previously added (you can see it in the video above) , do you want it added back?

p-j-smith marked this pull request as draft January 11, 2024 11:51

dstansby added this to the 2.0 milestone Jan 11, 2024

p-j-smith mentioned this pull request Jan 11, 2024

Use integer bins for integer data in HistogramWidget #244

Merged

p-j-smith force-pushed the feat/hist-bin-params branch from 9067a2d to 2ec6e7b Compare January 11, 2024 13:10

p-j-smith commented Jan 11, 2024

View reviewed changes

dstansby added New feature New feature or request Changelog needed labels Jan 11, 2024

p-j-smith added 8 commits January 15, 2024 15:14

Add widgets to set bin parameters for histograms

a7b09d4

Also set np.linspace dtype based on image dtype

Add test for histogram widget when setting bin parameters

96b2f0c

Update changelog

b8623ed

Make linters happy

4e4fb84

Fix type hints

5ac1f71

Don't allow bins lower than 0 if dtype is unisgned

fab2906

Update changelog

5553174

Undo changes to example of HistogramWidget

e86d4f6

dstansby modified the milestones: 2.0, 2.1 Jan 15, 2024

p-j-smith added 7 commits January 15, 2024 15:59

Fix autosetting bins from data

c5e0886

remove duplicate on_update_layers method

127d325

Make linters happy

b8ffdb7

Add HistogramWidget._bin_widgets attribute for storing bin widgets

88760cf

Fix calculation of bins from widget values

d56942b

Calculate step using n_bins-1

11590b2

n_bins corresponds to number of bin edges rather than bins

cannot use negative start bin for uint data

08a5086

p-j-smith force-pushed the feat/hist-bin-params branch from af10505 to 08a5086 Compare January 15, 2024 16:52

p-j-smith added 2 commits January 15, 2024 16:56

Make HistogramWidget bins_num widget correspond to number of bins rat…

65e84f8

…her than number of bin edges

Merge branch 'main' into feat/hist-bin-params

37d33ae

p-j-smith added 3 commits January 15, 2024 17:07

fix typo in comment about using 128 bins for float data

8a7d9e4

Merge branch 'main' into feat/hist-bin-params

fbd929e

Update changelog

7c4cdc8

p-j-smith marked this pull request as ready for review January 15, 2024 17:10

p-j-smith marked this pull request as draft January 15, 2024 17:12

p-j-smith commented Jan 15, 2024

View reviewed changes

src/napari_matplotlib/histogram.py Outdated Show resolved Hide resolved

p-j-smith marked this pull request as ready for review January 15, 2024 17:14

p-j-smith commented Jan 15, 2024

View reviewed changes

p-j-smith marked this pull request as draft January 15, 2024 17:26

p-j-smith added 2 commits February 14, 2024 10:51

Merge branch 'main' into feat/hist-bin-params

c6b5d8e

Add 'num_bins, 'start', and 'stop' parameters to '_get_bins'

6261f4c

p-j-smith marked this pull request as ready for review February 14, 2024 11:24

p-j-smith added 3 commits May 25, 2024 10:28

Merge branch 'main' into feat/hist-bin-params

457bc1b

use '| None' rather than Optional[Union[...]] for type hints

426a0f5

remove widgest to set start and stop values for histogram bins

67a8641

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add widgets for setting histogram bins #242

Add widgets for setting histogram bins #242

p-j-smith commented Jan 11, 2024 •

edited

dstansby commented Jan 11, 2024

p-j-smith commented Jan 11, 2024

p-j-smith Jan 11, 2024

p-j-smith Jan 11, 2024

p-j-smith Jan 11, 2024

p-j-smith Jan 15, 2024

p-j-smith Jan 15, 2024

p-j-smith Feb 14, 2024

dstansby commented May 25, 2024

p-j-smith commented May 25, 2024

Add widgets for setting histogram bins #242

Are you sure you want to change the base?

Add widgets for setting histogram bins #242

Conversation

p-j-smith commented Jan 11, 2024 • edited

dstansby commented Jan 11, 2024

p-j-smith commented Jan 11, 2024

p-j-smith Jan 11, 2024

Choose a reason for hiding this comment

p-j-smith Jan 11, 2024

Choose a reason for hiding this comment

p-j-smith Jan 11, 2024

Choose a reason for hiding this comment

p-j-smith Jan 15, 2024

Choose a reason for hiding this comment

p-j-smith Jan 15, 2024

Choose a reason for hiding this comment

p-j-smith Feb 14, 2024

Choose a reason for hiding this comment

dstansby commented May 25, 2024

p-j-smith commented May 25, 2024

p-j-smith commented Jan 11, 2024 •

edited