feat: Histogram Widget

[gselzer]

This PR adds a standalone widget for creating histograms of datasets. This PR is limited in scope, as requested by @tlambert03 - it does not interface with ndv in any way.

This PR is designed for integration with #51, and adopts a MVC structure to fit with the components being built over there. The components are described below:

StatsModel

This component is designed to retain computed statistics from a provided dataset (right now, it's just the histogram, really, but we may want more later), and signal listeners when the dataset changes.

StatsView

This component contains the API to describe an object that wants to listen for changes to the StatsModel, and update some display of that data for observation. One example might be a QWidget with some labels displaying mean and standard deviation, like exists in mmstudio.

LutView

This component contains the API to describe an object that wants to listen for changes to a LUT being used to visualize values in a dataset, and to update some display of that data for observation. The current ndv LutControl objects could be refactored into one of these. I believe that @tlambert03 has a LutModel as a part of #51 - we should coordinate here to make sure the API lines up.

HistogramView

This component is both a StatsView (because the histogram is a statistical feature of a dataset) and a LutView (because it has controls for clims, gamma, etc.), with some extra API pertaining to histogram viewing.

VispyHistogramView

This component is an implementation for HistogramView, backed by VisPy, and the real workhorse of the PR.

If you run x.py, you'll see the following, which is a VispyHistogramView that has been connected to a StatsModel. Right now, there is no controller, because 🤷

TODO

...I'm tired, but this will be an active list of wants/needs/dreams...

• Make sure things align with feat: ArrayDisplayModel (for later model/view refactor) #51
• Resolve TODOs
• Add vertical mode (+ API)
• Clean & Test (an interactive example that changes the data would be cool)
  • Test log Y
• PyGFX backend?

[codecov]

Codecov Report

Attention: Patch coverage is 92.65092% with 28 lines in your changes missing coverage. Please review.

Project coverage is 80.24%. Comparing base (10abe13) to head (e749c4b).
Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ndv/histogram/views/_vispy.py	91.79%	27 Missing ⚠️
src/ndv/histogram/model.py	96.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   78.93%   80.24%   +1.30%     
==========================================
  Files          13       16       +3     
  Lines        1856     2237     +381     
==========================================
+ Hits         1465     1795     +330     
- Misses        391      442      +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tlambert03]

hey! This is super cool.

I'm still in Italy, and will be slowly unburying myself after I return tomorrow. But will leave some scattered/disorganized thoughts here in the brief moments I have to look.

[tlambert03]

honestly, I think this is pretty great! I could certainly make tiny little nitpicky comments or suggestions, but they all feel rather bike-sheddy. I like the design a lot. There are perhaps a few usability quirks when using various methods like enable_range_log and other tiny little things that I came across, but i didn't write any of them down yet since they all felt rather insignificant on the whole. So, I'm very happy with the direction here. some open questions in my mind include whether most of this logic could love on some base class (kinda on the HistogramView Protocol object) that would implement anything that vispy or pygfx wouldn't directly need to care about, but i would just keep going with the vispy object for now as you are doing, and then look at refactoring out the backend agnostic stuff later.

since I don't have many concrete suggestions at the moment, you could just keep working on polishing things you know you wanted to work on. Or I could just keep playing with it and make tiny changes as I go that I could either push directly or in a sub-PR (sometimes that's easier for me than making github comments).

great work 🚀

---

edit: will add minor things I find here:

• when dragging the clims sliders, one shouldn't be able to drag one line "past" the other (i.e. clims[1] must be greater than clims[0])

[tlambert03]

in terms of alignment with #51, I'd say don't worry too much. I don't think that the LUTs signals are going to change all that dramatically; so whoever merges second can just make whatever little patches would be necessary if they diverge slightly

[tlambert03]

oh, and by the way, I do think we should look into seeing what fastplotlib can provide for the pygfx side of things. beyond the package itself, it brings no additional dependencies (beyond stuff we already depend on), so it could simply be added to the pygfx extra here

[gselzer]

[ ] when dragging the clims sliders, one shouldn't be able to drag one line "past" the other (i.e. clims[1] must be greater than clims[0])

@tlambert03 tell me what you think of 5a7097b.

[gselzer]

@tlambert03 any suggestions for resolving this FIXME? Without declaring them there, there's an infinite recursion caused by the interaction of vispy and psygnal whenever I try to access any of the LutView signals:

  File "c:\Users\gjselzer\micromamba\envs\ndv\Lib\site-packages\psygnal\_signal.py", line 370, in __get__
    setattr(instance, cast("str", self._name), signal_instance)
  File "c:\Users\gjselzer\micromamba\envs\ndv\Lib\site-packages\vispy\util\frozen.py", line 13, in __setattr__
    if self.__isfrozen and not hasattr(self, key):
                               ^^^^^^^^^^^^^^^^^^
  File "c:\Users\gjselzer\micromamba\envs\ndv\Lib\site-packages\psygnal\_signal.py", line 370, in __get__
    setattr(instance, cast("str", self._name), signal_instance)
  File "c:\Users\gjselzer\micromamba\envs\ndv\Lib\site-packages\vispy\util\frozen.py", line 13, in __setattr__
    if self.__isfrozen and not hasattr(self, key):
                               ^^^^^^^^^^^^^^^^^^

Note that the current situation draws complaints from ruff - the best fix I could think of is # noqa: B018 😅

[tlambert03]

I was already wondering whether using composition (rather than directly inheriting sceneCanvas) might work better, and I think that would maybe solve this too (but may add problems too, need to look closer)

[gselzer]

@tlambert03 I've made some significant changes here, including your composition suggestion, and created an interactive example within x.py - I'm going to try hacking on a pygfx example (will first look at fastplotlib as per your suggestion), but if you get a chance to look at the example I'd be interested in your thoughts

[gselzer]

oh, and by the way, I do think we should look into seeing what fastplotlib can provide for the pygfx side of things. beyond the package itself, it brings no additional dependencies (beyond stuff we already depend on), so it could simply be added to the pygfx extra here

Looking at fastplotlib, the library has a HistogramLUTTool, which is great, however:

• It doesn't take a precomputed histogram - maybe we could alter the design of the current StatsModel to just pass off the data, but I like the idea of precomputing the histogram.
• It's a bit buggy - for example, you can rotate the graphic like I do in that script, but the canvas events don't reflect that, so dragging clims behaves unexpectedly 😅
• No gamma curve is unfortunate.

Also, it's worth noting that the histogram does not provide the same customization as VisPy, which isn't a dealbreaker but is unfortunate.

I ran out of time to determine whether a logarithmic axis is possible - guessing so, but didn't have time to find it.

All of these points are based off of the latest version on PyPI right now - it's possible that the latest main has fixed many of these.

Starter example code below:

# test_example = true

from math import pi
import fastplotlib as fpl
from fastplotlib.widgets.histogram_lut import HistogramLUT
import numpy as np

figure = fpl.Figure(size=(700, 560))
# cmap_transform from an array, so the colors on the sine line will be based on the sine y-values
data = np.random.normal(10, 10, 10000).reshape((100, 100))
ig = fpl.ImageGraphic(data)
h = HistogramLUT(data, ig)
h.rotate(-pi / 2, "z")
g = figure[0, 0].add_graphic(h)
figure.show()


# NOTE: `if __name__ == "__main__"` is NOT how to use fastplotlib interactively
# please see our docs for using fastplotlib interactively in ipython and jupyter
if __name__ == "__main__":
    print(__doc__)
    fpl.run()

[tlambert03]

ok, that's great detail, thanks for looking into it. in that case, maybe we're better off vendoring/borrowing little bits more as a pattern of usage than a direct dependency

[gselzer]

maybe we're better off vendoring/borrowing little bits more as a pattern of usage than a direct dependency

I believe that general figure structure is still valuable and maybe worth the dependency, so I'm currently trying to hack together a Graphic subclass that can do what we want. Who knows, it may even be worth putting something together that could be contributed upstream?

[tlambert03]

just checking in here. here are things i think should happen before merge (and things that we don't need to do in this PR)

need

• the clims sliders shouldn't be allowed to pass each other, as mentioned above. I'm not quite sure what 5a7097b is doing behaviorally with newlims[:] = newlims[::-1]? I would expect the slider to just stop and not go any farther (and pin the newlims to the min/max of the other value accordingly)... but it kinda looks like you're auto-switching the grabbed handle to the other limit?
• need tests. I'm not worried about testing the visual/on-screen representation for now. but smoke testing all public methods for exceptions should happen. If you're worried about writing tests before we've finalized the patterns, that's fine, just add a couple basic ones (i think the protocols themselves are unlikely to change and could be tested)
• linting and pre-commit
• open question: I wonder whether _clim_handle_grabbed and _gamma_handle_grabbed should be combined into a single grabbed_item concept, that has an Enum of possible things to grab:

  class Control(Enum):
      LEFT_CLIM = auto()
      RIGHT_CLIM = auto()
      GAMMA_HANDLE = auto()

  that would be nice, but presumes we can only ever grab a single thing at a time (which i think makes sense?)
• general cleanup and document all public methods, most importantly in the _view.py protocols

at that point, I think I'd like to take a finer-tooth comb through it and make direct edits for a PR to your PR

don't need here:

• pygfx backend. We can do that later, and until then we can simply refuse to show a histogram in the pygfx version.
• Qt (or any other front-end) wrapper. I think you can just remove the QtHistogramView. For the example, just use VispyHistogramView.view(), and you can add those additional buttons in some example if you want, but i don't think we (yet) need to have that kind of object in the library.
• don't need to hook this up to the viewer yet. let's do that in another pr. This should just introduce a stand-alone widget.

[gselzer]

• I would expect the slider to just stop and not go any farther (and pin the newlims to the min/max of the other value accordingly)... but it kinda looks like you're auto-switching the grabbed handle to the other limit?

Yup, this is what I'm doing. I'm not sure what you mean by pinning the newlims to the min/max of the other value, and you could definitely convince me to change the behavior if you can maybe elaborate a bit more on what you want to do and why.

The situation I'd like to avoid is struggling to grab the desired handle when the sliders overlap (this is what happens, for example, in the fastplotlib histogram). It's possible that we could solve this, as well as implement the behavior you're interested in, by making explicit handles that are dragged instead of the line, like what mmstudio offers.

need tests. I'm not worried about testing the visual/on-screen representation for now. but smoke testing all public methods for exceptions should happen.

👍 (also goes for testing/documentation/cleanup)

that would be nice, but presumes we can only ever grab a single thing at a time (which i think makes sense?)

The only situation where I could see wanting to grab multiple things at once is if you wanted to "translate" the clims left/right (i.e. "grab both"). But we could make a separate enum for that, because you'd presumably want to specify that window with a separate object (like fastplotlib does).

• pygfx backend. We can do that later, and until then we can simply refuse to show a histogram in the pygfx version.

Sure, I'll throw my current efforts onto a new branch.

• Qt (or any other front-end) wrapper. I think you can just remove the QtHistogramView. For the example, just use VispyHistogramView.view(), and you can add those additional buttons in some example if you want, but i don't think we (yet) need to have that kind of object in the library.

That was actually mostly created as a visual test of the widget, and my plan (aligning with your suggestion) was to move it to an example. Good to have an explicit reminder though!

• don't need to hook this up to the viewer yet. let's do that in another pr. This should just introduce a stand-alone widget.

👍

[gselzer]

@tlambert03 I added handles for adjusting the clims in 89161c4. I think that the Markers could use some manipulation, but this would solve both my frustration of being unable to distinguish between left and right clims when they are overlapped, and your frustration of the clims surprisingly "sliding" past each other. I'm not tied to the idea, but let me know what you think!

[gselzer]

Did a large amount of documentation/testing/cleanup in 0090abc. Still lots to fix here (including the missing dependencies, which I should have just added in this PR (hadn't yet due to the threat of merge conflicts in #51)

[gselzer]

• it might be possible to combine the 4 nodes (histogram and handles) into a single vispy.scene.Compound widget, and get rid of some code... but then you'll still need to figure out how to have a different transform for the handles than that histogram. so I'm 100% fine with the way you've done it here.

If you're fine with it, let's just leave it the way it is then 😅

• I think i do prefer the old clim-grabbing behavior (before the circle handles at the top and the bottom). While it's nice that they are unambiguous when on top of each other, they're too hard to grab... I want to grab the line itself. I would not mind at all if, when the two lines are on top of each other, you always just get the clim-max line (95% of the time, I suspect that's what someone will be moving... and if it wasn't what they wanted, they can always zoom in to disambiguate)

Rewrote history to remove that behavior, I tried my best to ensure your formatting changes made it through.

[tlambert03]

great, thanks for that reversion. i'm noticing now that the tolerance of 3 for whether a handle is grabbed or not appears to be chosen in data-space, rather than canvas/monitor-pixel space. That will make for problems with small ranges, e.g. a float histogram between 0-1. see below, where line is on 20, and mouse is around 17:

Untitled.mov

[tlambert03]

couple more things I noticed here while playing with this:

• clim handles don't appear to work in vertical mode, but the gamma handle does.
• though it might seem a little confusing, I think the Y axis should be inverted in "vertical" mode: with negative values at the top, and positive values at the bottom (making it a 90˚ clockwise rotation of the horizontal plot). alternatively, the y axis could be on the right side, making it a 90˚ counter-clockwise rotation of the horizontal plot.
• in logarithmic mode, the Y axis also needs to be logarithmic, so that the apparent range of values doesn't change (i.e. if the peak value in the regular histogram is ~130, then it should still be 130 in logarithmic mode). that means we can't just apply the logarithm to the counts themselves without modifying the axis as well. Not sure if vispy supports this easily...

in all cases, I'm happy to just merge this now and have them fixed in a follow up PR. I've just invited you to the repo with write access, so feel free to merge this if you want to be done with it and move on to smaller incremental fixes :)

[gselzer]

in all cases, I'm happy to just merge this now and have them fixed in a follow up PR. I've just invited you to the repo with write access, so feel free to merge this if you want to be done with it and move on to smaller incremental fixes :)

Nah, let's clean them up now, while we're still thinking about it:

• clim handles don't appear to work in vertical mode, but the gamma handle does.

Fixed in 71edf3c

• though it might seem a little confusing, I think the Y axis should be inverted in "vertical" mode: with negative values at the top, and positive values at the bottom (making it a 90˚ clockwise rotation of the horizontal plot). alternatively, the y axis could be on the right side, making it a 90˚ counter-clockwise rotation of the horizontal plot.

Fixed in d5b4b6a

• in logarithmic mode, the Y axis also needs to be logarithmic, so that the apparent range of values doesn't change (i.e. if the peak value in the regular histogram is ~130, then it should still be 130 in logarithmic mode). that means we can't just apply the logarithm to the counts themselves without modifying the axis as well. Not sure if vispy supports this easily...

I guess I'm unsure what you mean by this - can you elaborate? Ah, are you talking about applying a logarithmic scale to the axis itself, such that you still see the actual counts in each bin? If so, then yeah, I don't think vispy really supports that - I remember looking a bit.

EDIT 2: See here for proof that it isn't supported 😅. EDIT 3: And more proof. Maybe we should try to push a fix upstream?

[tlambert03]

such that you still see the actual counts in each bin

yeah this. that's fine. future goal :)

EDIT 2: See here for proof that it isn't supported 😅. EDIT 3: And more proof. Maybe we should try to push a fix upstream?

sure ...future :)

[gselzer]

such that you still see the actual counts in each bin

yeah this. that's fine. future goal :)

EDIT 2: See here for proof that it isn't supported 😅. EDIT 3: And more proof. Maybe we should try to push a fix upstream?

sure ...future :)

Until then, would you prefer axis text?

[tlambert03]

tests have broken (in case you didn't notice)

[gselzer]

tests have broken (in case you didn't notice)

Ahh, sorry, I thought I had pushed the fix but instead it got caught up in a local-only WIP commit that added those axis labels in my screenshot above. Pushed now.

[kushalkolar]

Hi! Cool to see you guys using pygfx & fastplotlib 😄 . I just thought I'd add some thoughts. I want to refactor our histogram tool soon due to issues like fastplotlib/fastplotlib#673, fastplotlib/fastplotlib#645, so this is good timing!

The current HistogramLUTTool was mainly created for the [ImageWidget](https://www.fastplotlib.org/ver/dev/_gallery/image_widget/image_widget.html#sphx-glr-gallery-image-widget-image-
widget-py) so we did not have more general use cases in mind when we wrote it. Nonetheless I want to refactor it into something more reliable and versatile.

Log scales is on the roadmap for pygfx, but it will be a while before that's ready. The next release of fastplotlib will have basic axes, and one for the histogram too in a future release.

I'm currently trying to hack together a Graphic subclass that can do what we want.

I'm curious what additional functionality you require!

I notice you subclassed the PanZoomController to restrict it to a single axis, you can already do this and more with the PanZoomController. See PR: pygfx/pygfx#778 (also related issue: pygfx/pygfx#635), example1, example2

Unrelated to this PR, I notice you're also working on ROI tools with pygfx, we've got a RectangleSelector but we might take some of what you guys come up with if you have plans for a polygon and lasso tool.

[tlambert03]

thanks for checking in here @kushalkolar! :) much appreciated

[gselzer]

Log scales is on the roadmap for pygfx, but it will be a while before that's ready.

Oh, that's great!

I'm curious what additional functionality you require!

It's mostly just the things I described here:

Looking at fastplotlib, the library has a HistogramLUTTool, which is great, however:

* It doesn't take a precomputed histogram - maybe we could alter the design of the current `StatsModel` to just pass off the data, but I like the idea of precomputing the histogram.

* It's a bit buggy - for example, you can rotate the graphic like I do in that script, but the canvas events don't reflect that, so dragging clims behaves unexpectedly 😅

* No gamma curve is unfortunate.

The precomputed histogram feature is the one that's really important for us - the others would likely be simpler inclusions.

I notice you subclassed the PanZoomController to restrict it to a single axis, you can already do this and more with the PanZoomController. See PR: pygfx/pygfx#778 (also related issue: pygfx/pygfx#635), example1, example2

This is also good information, I'll have to take a look!

Unrelated to this PR, I notice you're also working on ROI tools with pygfx, we've got a RectangleSelector but we might take some of what you guys come up with if you have plans for a polygon and lasso tool.

Yes, moving things upstream would be great. I know ROI that has implications for the histogram is a line ROI, as in MicroManager we often want to compute histograms on points along a line - I didn't see that on your issue, but maybe it's worth adding?