WIP: use scenex#276
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #276 +/- ##
==========================================
+ Coverage 85.38% 88.58% +3.20%
==========================================
Files 53 42 -11
Lines 7192 4942 -2250
==========================================
- Hits 6141 4378 -1763
+ Misses 1051 564 -487 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Since we aren't running within a notebook, vispy will try to use glfw over jupyter unless we force jupyter with the environment variable. This is the same thing we do in scenex
Needed to get SignalGroupDesciptor.connect_child_events
This avoids some scenex errors from casting very large values into 32-bit floats
Will probably move them to a different file later
The few functions that still have use have been moved to src/ndv/views/_util
Just makes for an easier diff
| mock_highlight.assert_called_with({}) | ||
|
|
||
|
|
||
| # ---------- Visual behavior tests ---------- |
There was a problem hiding this comment.
Noting that these tests were moved over from tests/views/_vispy/test_shared_histogram.py
| @@ -1,279 +0,0 @@ | |||
| """Tests for VispySharedHistogramCanvas visual behavior.""" | |||
There was a problem hiding this comment.
These tests were moved to tests/test_shared_histogram.py
| @@ -1,168 +0,0 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
This is really a scenex concern, see how these things are tested e.g. here
| @@ -1,163 +0,0 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
These tests were moved to test/test_histogram.py
I think all of these have scenex equivalents
| """Whether to use %gui magic when running in IPython. Default True.""" | ||
|
|
||
|
|
||
| class NDVApp: |
There was a problem hiding this comment.
Note that it would be nice to just get rid of this class, however it's public API
| from ._view_base import Viewable | ||
|
|
||
| __all__ = [ | ||
| "ArrayCanvas", |
There was a problem hiding this comment.
Just noting that there's lots of public API here being deleted already
| "CanvasBackend", | ||
| "GuiFrontend", | ||
| "call_later", | ||
| "get_array_canvas_class", | ||
| "get_array_view_class", |
There was a problem hiding this comment.
More public API being deleted
| @@ -1 +1 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
This is another file ripe for pruning.
|
@tlambert03 I think I'm getting happier with how this is looking, and would be interested in your initial thoughts here. As I mentioned above, this is a rather large amount of changing code, and it would need a release of scenex before merge. More than anything else, I'm interested in your thoughts about the effects to the project structure, including:
Despite all of that removal, I think all ndv features are fully functional (let me know if any testing you do uncovers anything I missed). |
1 participant
Overview
This PR aims to replace the references to multiple canvas backends (vispy, pygfx) with a single implementation against scenex models. It's a large change, deserving lots of review and scrutiny.
Reasoning
One canvas implementation The flexibility ndv offers users presents significant effort for ndv developers who want to enable a feature across both canvas implementations. Both
src/ndv/views/_vispyandsrc/ndv/views/_pygfxhave thousands of lines of code and separate test bases. As this PR shows, adopting scenex results in far fewer raw lines of code to maintain.Robust scene models Adding more functionality (including the ability to display multiple datasets, multiscale data as suggested in multiscale chunked rendering #275, orthoviews, etc.) will require knowledge of the scenegraph state. There are already bits and pieces of this in ndv (e.g.
ImageHandle,RectangularROIHandle), but scenex exists for this purpose.Implementation Goals
The primary goal of this PR is to condense down to a single canvas implementation, removing dependencies on vispy and pygfx in favor of dependencies of scenex, while maintaining the same public API, and all (excluding tests aimed at removed behavior) tests.
@tlambert03 your review is always welcome, but might be good to wait a little longer, as I fear that despite passing tests on my machine this PR will show me that there are many more things to clean up!