feat: v2 histogram (simplified alternative) #65
Conversation
Wraps the VispyHistogramView up with some Qt widgets for control. Derived from the histogram example
More view refactoring is likely necessary. A PStatsView might just want to accept the dataclass, if we even see a need to maintain that view protocol.
I don't think that API needs to be accessible from the UI (by default)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2-mvc #65 +/- ##
=========================================
Coverage ? 66.41%
=========================================
Files ? 33
Lines ? 3490
Branches ? 0
=========================================
Hits ? 2318
Misses ? 1172
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
it's hooked up to the view and shows up in the main viewer now if you want to run
mvc.pyas an entry point
Looks nice! For some reason it suffers from pretty poor performance on my machine, though - but I think it has nothing to do with this rework, as I saw the same things on #55.
- the main frontend ViewerView object now accepts a histogram widget (just like it accepts a canvas widget) and can place it wherever it wants in the view. For now that breaks pygfx, which doesn't have a histogram... so we probably need to come up with a slightly "lazier" pattern using something like
add_histogramto the view, rather than passing in init.
- as mentioned elsewhere, we need to figure out how to deal with multiple lookup tables on the same histogram widget. Currently it's just showing the "last" channel that was updated. so, in composite mode, it's only showing the last channel
I was thinking that, instead of accepting a histogram widget, the histograms should be created (on demand?) by/along with the LUT widgets. I had some other ideas on implementation here, too, but was going to wait for #55 to be merged before I tried.
- similarly, we need a way to turn the computation for the histogram off when not needed
Yes, good point. This was one thing I liked about the Stats dataclass I had in #55, which computed all of these statistics lazily. If we had that, then this would be as simple as adding some visibility check within the histogram.
Also looks like the Jupyter frontend isn't quite working yet?
src/ndv/controller/_controller.py
Outdated
| # TODO: once data comes in in chunks, we'll need a proper stateful | ||
| # stats object that calculates the histogram incrementally | ||
| counts, bin_edges = _calc_hist_bins(data) | ||
| self._histogram.set_data(counts, bin_edges) | ||
| # self._histogram.set_range() |
There was a problem hiding this comment.
The stats object would also be nice for sharing computational results, right? For example, it's probably helpful in autoscaling done by lut_ctrl.
Curious as to why you removed the Stats dataclass given the reasons that we will likely want it...
There was a problem hiding this comment.
i removed it because, at the moment, it's not necessary. Once we start seeing what will actually consume it then it will be clearer what roles it needs to fill. So, basically: need-based design rather than thinking ahead about what might be useful in the future.
yeah that's what I meant by "so we probably need to come up with a slightly "lazier" pattern using something like add_histogram to the view,"
we could go back and merge #55, that would be fine with me ... but i think it needs to have the qtview jupyter view and stats objects removed... they're too much for now |
would be good to run a profiler on your machine to see which functions are eating the most time |
That's fair, and it's easy enough to iterate on these things later. LGTM so we can continue iterating on more things! |
implemented now
working again now, but doesn't yet show histogram |
it's also as easy as just doing |
|
ok, I'm going to merge this after tests pass. We need to just have the vispy histogram in the main v2 branch so we can both move on and continue to play with it. Thanks for all your work, ideas, and patience on this! |
hey @gselzer. this is what I came up with after rearranging #55
Screen.Recording.2024-12-10.at.8.26.39.AM.mov
note: cursor changes actually do work... but apparently not in the screen recorder
it's hooked up to the view and shows up in the main viewer now if you want to run
mvc.pyas an entry pointsharp bits that still need to be considered:
add_histogramto the view, rather than passing in init.