Skip to content
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

feat: add pygfx-backed histogram #105

Merged
merged 16 commits into from
Jan 22, 2025
Merged

Conversation

gselzer
Copy link
Collaborator

@gselzer gselzer commented Jan 17, 2025

This is a repeat of #76, ready for eyes again.

There are a few problems I'm aware of, which I'm tempted just solve in separate PRs:

  • There's "shimmering" when you pan/zoom the histogram canvas, due to aliasing. PyGFX's antialias solution of renderer.pixel_ratio was not sufficient to fix this on my machine.
    • Note that the VisPy histogram also has this issue, but it is only seen when zooming.
  • When you change the data, the pan/zoom resets. This is also a problem for the VisPy histogram.

Many things still don't work. But some things do!
Was previously using the bounding box maximum which was the height of
the clims :(
@gselzer gselzer added the enhancement New feature or request label Jan 17, 2025
@gselzer gselzer requested a review from tlambert03 January 17, 2025 23:07
@gselzer gselzer self-assigned this Jan 17, 2025
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 56.15942% with 121 lines in your changes missing coverage. Please review.

Project coverage is 75.69%. Comparing base (133f7c1) to head (f39c5fc).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/ndv/views/_pygfx/_histogram.py 55.83% 121 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
+ Coverage   72.43%   75.69%   +3.26%     
==========================================
  Files          48       49       +1     
  Lines        4622     4897     +275     
==========================================
+ Hits         3348     3707     +359     
+ Misses       1274     1190      -84     

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

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completely works for me!

nit re fps

# closes on the event filter and keeps it in scope).
self._disconnect_mouse_events = filter_mouse_events(self._canvas, self)

self._renderer = pygfx.renderers.WgpuRenderer(self._canvas, show_fps=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe remove show_fps or guard it behind an env var or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch - will remove, I'd rather more thoroughly address that across the repo in #47

@tlambert03
Copy link
Member

added a basic smoke test

@tlambert03
Copy link
Member

one test still consistently failing with pygfx and pyside on windows: https://github.com/pyapp-kit/ndv/actions/runs/12856369561/job/35842981229?pr=105

not 100% sure it's the histogram test, but it seems like it's happening right about the same time

@tlambert03
Copy link
Member

was able to reproduce the test segfault on a real windows 3.12, pyside, pygfx about 50+% of the time. So I do think it's a real bug. But since we're already only partially supporting pyside, let's go ahead and skip it

@tlambert03
Copy link
Member

i'm going to merge this, but I think the additional test I added here, though just a simple smoke test, is causing more random test failures. It could be two things: either something related to the histogram test itself, or simply related to the fact that the new test is actually the first test added that (re-)uses the backend application for a second time. That does come with a whole host of possible gotchas. But, since that's unrelated to this PR, and since we were likely to hit it eventually anyway, I'm going to merge this, and we may have to deal with a brief period of test flakiness while I figure that out.

thanks for your work here!

@tlambert03 tlambert03 merged commit 796588a into pyapp-kit:main Jan 22, 2025
51 of 52 checks passed
@tlambert03 tlambert03 changed the title Pygfx histogram v2 feat: add pygfx-backed histogram Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants