- Notifications
You must be signed in to change notification settings - Fork22
Add widgets for setting histogram bins#242
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR! Could you put the integer dtype histogram change into a separate PR to make it easier to review? |
Yep, will do 🙂 |
9067a2d to2ec6e7bComparesrc/napari_matplotlib/histogram.py Outdated
| bins_start = QDoubleSpinBox() | ||
| bins_start.setObjectName("bins start") | ||
| bins_start.setStepType(QAbstractSpinBox.AdaptiveDecimalStepType) | ||
| bins_start.setRange(-1e10, 1e10) |
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 wasn't sure what a reasonable range would be (rather than the default of 0 to 100)?
src/napari_matplotlib/histogram.py Outdated
| # 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) |
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.
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)
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.
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
af10505 to08a5086Comparesrc/napari_matplotlib/histogram.py Outdated
| # 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) |
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.
temporarily disable the callbacks otherwise the plot is re-drawn each time the widget value changes
Uh oh!
There was an error while loading.Please reload this page.
src/napari_matplotlib/histogram.py Outdated
| 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) |
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 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?
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've added the notification now, but happy to remove it and add separately if it's easier
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 futureif 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 |
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? |
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.
Sorry this has taken so long to get to, but I gave it a try and it looks 👍👍👍👍 . I've mergedmain into here, and will update the changelog entry, then get this in a release 😄
251d333 intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
no worries 😄 Thanks for the reviews and for merging it! |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#239
QDoubleSpinBoxwidgets for setting the start and stop edges of the histogram bins (default to the min and max data values, same as existing values used)QSpinBoxwidget for setting the number of bins to use (defaults to 100, same as the existing value used)set the- this is now done inUse integer bins for integer data indtypefornp.linspacewhen creating bins (so integer bin limits are used for integer images). This caused thetest_histogram_2Dtest to fail (as the test layer has integer data but float bins were being used) so I've updated the baseline imageHistogramWidget#244HistogramWidgetwhen the bin parameters are set from the widgetsstart,stop, andnum_binsparameters to_get_binsfunction so that bin parameters set in the widgets can be used for settings binsnapari-mpl-hist-bins.mov