-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Better histogram control (plus pygfx histogram overhaul) #146
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Not much utility to a slider
Makes log work, but more importantly makes for better histogram interaction and cleaner logic
Will save for another PR
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #146 +/- ##
==========================================
+ Coverage 80.80% 84.37% +3.57%
==========================================
Files 45 45
Lines 4271 4487 +216
==========================================
+ Hits 3451 3786 +335
+ Misses 820 701 -119 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
also great stuff here. if implementing the slider was problematic, i'm fine with a button for now. I was recently playing with histograms as well, and trying to apply range limits (so that the thing doesn't scroll past, for example, 0-65,535). I think it's pretty straightforward using the 1DPanZoom camera, and updating the I'll have a closer look at the changes required here soon. thanks! |
Useful for histograms right now but potentially for array canvases as well?
Let me know what you think of 5bf0f1e - could use a little polish but I think it works pretty well. |
I don't think this applies anymore
Since the goal is to not move the camera when updating data, without this change we'll never set the camera to the right place the first time!
8621b37
to
6c79110
Compare
6c79110
to
f9500d1
Compare
Not sure how I feel about this addition to the model, but it's very useful!
Particular review going to acedb6e would be worthwhile, as it adds a new field
as well as #152, which this PR would now close. However there may be a better place to store this state... |
...maybe this can resolve the macos segfaults?
12db99a
to
e5ee529
Compare
e5ee529
to
48f8c90
Compare
Still getting widget leaks...at least the test passes...
15f8cd4
to
cf5f19c
Compare
It's not clear how to fix them...
@tlambert03 I think this PR is probably ready for some eyes, in particular I'd like your feedback on:
|
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.
hey @gselzer,
i took a brief look at this in the time I just had available and I like everything I see. I'm liking everything i see, and as usual: all your changes indicate a deep understanding of the system and they're all for the better. since I'm a bit busy this week, if you want to just merge this at any point, feel free to, (or specifically ask anything else). I'm happy with clim_bounds, and 5bf0f1e looks fine to me.
haven't looked too deeply at the pygfx changes. but the behavior seems good (outside of a sizing issue as shown in the screenshot below when using histogram in pygfx... i think it might need an explicit size policy?)

I'll take a look at the leaks when I have a moment (but skipping them now is also ok with me)
Should solve histogram taking up all of the vertical space...
This PR adds:
To accomplish these features, some serious TLC was required to accomodate the new histogram features using pygfx. The way that we were calculating the margins was becoming increasingly complicated, so I developed a new strategy where the plot, x-axis, and y-axis are all rendered separately. This enables us to easily:
The downside here is more complication in the rendering pipeline, but I think it's better than it was.
Recording.2025-02-28.213811.mp4
These controls should be ready for opinions (cc @tlambert03, @fdrgsp), but also still need to:
FIXME
s