napari-matplotlib icon indicating copy to clipboard operation
napari-matplotlib copied to clipboard

Add widgets for setting histogram bins

Open p-j-smith opened this issue 1 year ago • 2 comments

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 #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)

https://github.com/matplotlib/napari-matplotlib/assets/29753790/876ab0e9-0b46-4efd-b408-c4ffa184d799

p-j-smith avatar Jan 11 '24 11:01 p-j-smith

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

dstansby avatar Jan 11 '24 11:01 dstansby

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

Yep, will do 🙂

p-j-smith avatar Jan 11 '24 12:01 p-j-smith

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

dstansby avatar May 25 '24 08:05 dstansby

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 avatar May 25 '24 10:05 p-j-smith

Sorry this has taken so long to get to,

no worries 😄 Thanks for the reviews and for merging it!

p-j-smith avatar Jul 12 '24 14:07 p-j-smith