feat: ArrayDisplayModel (for later model/view refactor) #51
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
===========================================
- Coverage 78.93% 56.00% -22.93%
===========================================
Files 13 22 +9
Lines 1856 2430 +574
===========================================
- Hits 1465 1361 -104
- Misses 391 1069 +678 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@tlambert03 this is a great start, thanks for making it public so quick!
reviewed for soundness of the model itself
I think that the model is sound, but am wondering about your plans for modeling the other data involved, namely the array data itself (which right now is just a wrapper, which might be fine) which might have associated metadata (like pixel scale, offsets) relevant to the display, as well as ROIs.
It's also worth denoting whether this model could handle multiple datasets (which is a big can of worms we can address later, but it's still worth thinking about for model robustness). I think that, written right now, it could, but is that a "feature" we should preserve later, or just an aspect of the current state it maintains?
interactive validation (i.e. instantiate the object and see if you can break it with invalid inputs)
Buggy things like this could be improved later, but fwiw I don't think m.current_index["5"] = slice(1, "fooo") should be allowed 😆.
signal emission
Don't have much to say here, other than that I'd love to try hooking this up to a widget and see how far I/we can get.
just pushed some of my recent playground changes (in
yes that is definitely a goal here (and is part of why i exclude the dataset from the display model). I don't think it's a big can of worms really... it just requires a more explicit model of a "world object" (i.e. a thing that has a data-to-world transform) that tells us how to place the object in the scene graph. Essentially, right now you can think of it as everything having an (implicit) data-to-world transform of "identity". basically, i think everything's still on a good path here for these things
see x.py for now in this PR. Note, at this point it's just in "hook all the things up mode" ... not clean and not refactored to be clear |
the only issue there is the "fooo" in the slice right? I can update the Slice type validator |
…at are not integers
Oh, great! Since you're already working on the "view" part...how much are you planning to separate the "view" part (Qt/Jupyter, vispy/pygfx) from the "controller" part (signal connections, maybe some top-level user API)? Might be helpful in resolving #48
Glad to hear it! Since we're talking about transforms, do you think it's worth allowing
❤️
Yeah, think so! |
was definitely also in scope for this :) i just split some stuff up and now there's an |
Gorgeous 😍 this was exactly what I was envisioning, and I was hacking out something like this before I saw your WIP commit yesterday. |
|
great! makes me happy to hear it's along the lines of what you were picturing |
|
something I'm still thinking about a lot (as you were above) is the relationship between the data wrapper and the display model. I do definitely want it to be possible to have the same display model associated with two different data wrappers (this is essentially like "synced windows" in Fiji, where you link the current index and luts of two different images). And I also want it to be possible to associate two different display models with the same data wrapper (this would make orthoviewers trivial). So I don't think I want to simply add a class DisplayedData:
data: DataWrapper | None
view_model: ArrayDisplayModelThat object would have logic like the |
this concept is important if we want to select a display in world space (instead of data space), but that sort of thing was such a hairy ball of wax in napari, that I want to leave all that kind of multi-world-object negotiation for another PR |
It's possible that I just haven't dug as deep in as you have yet, but why can that not be resolved in the view, or in the controller? I do like the idea of thinking about the "Viewer" having three things, which it is responsible for interconnecting:
|
|
I've also been hacking more on #49 in the meantime, now off of your prototype. Do you think that it makes sense to build up To be clear, I'm not suggesting we do these things in this PR - if this is something we want, I'll build it separately, working for the current |
I do actually think that model view could work there. That's a better way to phrase the vague things I was saying about some "stats service" (i.e. model), the thing that can calculate min max mean histogram, etc... once, and be shared by various things, like the auto-scaler, the histogram, etc... |
|
Yup, and I also like the idea of a view protocol because it could make it easier to define multiple histogram widgets as I describe in #49 - a simple, small one that could replace dimension sliders, and a more complicated widget like you had in napari/napari#675 that could be brought up with double click... |
|
proof of principle working with Jupyter :) Untitled.mov |
| class DataDisplayModel(NDVModel): | ||
| """Combination of data and display models. | ||
|
|
||
| Mostly this class exists to resolve AxisKeys in the display model | ||
| (which can be axis labels, or positive/negative integers) to real/existing | ||
| positive indices in the data. | ||
| """ |
There was a problem hiding this comment.
I think it's unfortunate that this class is necessary - it starts to tie together one display model to one dataset, which could restrict our ability to display multiple datasets using a single data model. I'd like to look more into whether this is really necessary (I'm guessing it's hairy since you had to make this, but maybe a fresh set of eyes will help?)
There was a problem hiding this comment.
Yeah that class is still up in the air for me too, it may be removed... it exists independently specifically because we do kinda need one place to tie one model (which has abstract axis keys that may or may not mean anything) to one dataset (where those keys are canonicalized into actual dimension indices or errors). And this class does that. For example if you had a model that says `visible_dims = ('t', 'x', 'y'), but a dataset comes along that has no t, we need to resolve and check that somewhere
|
|
||
| self._set_model_connected(self._dd_model.display) | ||
| self._view.currentIndexChanged.connect(self._on_view_current_index_changed) | ||
| self._handles: dict[int | None, PImageHandle] = {} |
There was a problem hiding this comment.
Could these handles be a property of the view?
I'm guessing that they're here because the view needs to be passed the data contained in the DataDisplayModel, but what if the data was instead a property of the view?
There was a problem hiding this comment.
They're actually there more because it's vispy/pygfx that provide the handles, and I'd like to be able to use that same logic regardless of the front end (qt/jupyter) view.
But this is a good thing to flag 👍, this isn't a pattern I'm married to at all
| # TODO: we need a way for DataWrapper to be able to say that the dims/coords | ||
| # have changed (essentially, that the shape of the data has changed) | ||
| # among other things, this would allow the canonicalized_axis_map to be cleared |
There was a problem hiding this comment.
I think in a lot of ways the DataWrapper is in and of itself a Model class - if it was a DataModel instead, things could listen to the data changing?
There was a problem hiding this comment.
Yeah, it's model-like for sure. And yes, it needs some signals. Namely dimsChanged and perhaps coordsChanged. dataChanged itself is really quite hard (something we struggled with in napari a lot), because it can be very hard to spy on in-place mutations for many array types. But it would still be good to have a signal there that could at least be manually emitted
| self._visible.value = visible | ||
|
|
||
|
|
||
| class JupyterViewerView: |
|
to prevent confusion, I'm going to close this PR. As I told @gselzer, this branch is now present upstream at https://github.com/pyapp-kit/ndv/tree/v2-mvc ... and to facilitate multiple people (me and gabe) working on different aspects of it at the same time, all PRs working on the model view refactor will be made to that |
this is the model-only partly from #33, cleaned up for easier review. This isn't yet connected to a view, but can be reviewed for soundness of the model itself, interactive validation (i.e. instantiate the object and see if you can break it with invalid inputs), and signal emission (they should be appropriate for later view connection).
see the
display_model.pyexample for a starting pointcc @gselzer if you want to play with it and provide feedback (including requests for more docstrings & comments for any parts that seem confusing or magical), or try to connect it to a view