Add NAP-3: Spaces

[brisvag]

Description

Following the discussion in #4419, here's a condensed version of the spaces proposal, in NAP form.

I hope I addressed most comments in the original discussion; if not, please let me know and I will update the NAP by extending the Alternatives section (or whatever else is appropriate).

Note that, while I made a point against the "multiple viewers" approach based on nomenclature, I don't feel strongly about the "spaces" name, so you're welcome to propose anything more intuitive/better sounding than that :)

NAP away! 😴

[alisterburt]

Thanks @brisvag - approving so that this can be merged at earliest convenience for discussion!

I'm 100% in agreement that we need this feature but not so sure on the proposed implementation. To me, the top level app interface feels cleaner and better supports both 'spaces' (multiple viewers in one window) and 'multiple viewers' (multiple viewers in multiple windows). It also signifies a cleaner API break which might be a benefit for such a large feature. I would propose that we go that way but let's see what others say :-)

See #4419 for more discussion around the top-level app API

[Czaki]

For me, current NAP text sounds more like "states", not "spaces"

[brisvag]

To me, the top level app interface feels cleaner and better supports both 'spaces' (multiple viewers in one window) and 'multiple viewers' (multiple viewers in multiple windows).

Can you elaborate on this a bit, so I can add a proposed API in the alternative section? Something like I did above for the proposed api would be nice, so we know exactly where everything would live. At present, I can imagine something like:

app.viewers
app.viewer   # active viewer
app.spaces
# spaces are viewer-independent

Or something along those lines, which to me feels a lot clunkier, while effectively doing the same thing as this proposal. I don't generally see the benefit of this extra app step, other than, as you mentioned, signaling a clear break. But why do we need that, if the change can be backward compatible?

Or do we want to anticipate the api break to now, expecting that in the future we'll want spaces to be viewer-independent, and be able to show the same space in different viewers? To me it seems that this should be a multicanvas thing, rather than multi-viewer. What use case wants the same data in separate windows but would be unhappy with the same data in easily-switchable spaces in the same window, or else in two separate side-by-side canvas?

[brisvag]

For me, current NAP text sounds more like "states", not "spaces"

That's a reasonable name as well. While I suppose states is technically more correct, I personally find it a bit too generic, compared to spaces, which conveys the idea better to me. I would like to hear what others think as well!

[kevinyamauchi]

Super cool NAP, @brisvag !

I haven't had a chance to dive in yet, but I saw the discussion about the name and I wanted to quickly mention that I would prefer something other than "spaces". While I agree "spaces" is pretty nice, I have been seeing spaces popping up in various places (e.g., OME-NGFF and some discussions here in napari) to describe set of axes for a coordinate system. For example, in napari, the layer data coordinate system would be a space and the global coordinate system would be a different space. Personally, I like "space" better for describing coordinate systems and if possible, would prefer to reserve it for that.

I agree with @Czaki that "states" feels more "technically correct", but it may be too jargoney. I'll give it some thought and see if I think of anything else.

[alisterburt]

Can you elaborate on this a bit, so I can add a proposed API in the alternative section? Something like I did above for the proposed api would be nice, so we know exactly where everything would live. At present, I can imagine something like:

What I imagined is more along the lines of

app.viewers
app.viewer   # active viewer
app.window # active window

could also add app.windows for multiple windows 🙂

This clearly separates out the application (the thing running the event loop) from the windows which may contain one or more viewers (your spaces). I don't think the viewer==window association is that strong, I think the familiarity is more with the Viewer API.

The hierarchy just feels a bit simpler to me with a top level thing to which everything else belongs I think, don't have to think about it so much

[andy-sweet]

If I understand the NAP process, this looks good to me as a draft.

I made some typo/grammar-bot comments/suggestions, which would be to fix before merging. I also had some questions, which are optional for now, but probably need to be answered at some point.

[andy-sweet]

Suggested change

	Additionally, there are no benefits to keeping multiple `ViewerModel` objects alive, since their purpose is simply to act as an evented model to update GUI.
	Additionally, there are no benefits to keeping multiple `ViewerModel` objects alive, since their purpose is simply to act as an evented model to update the GUI.

[andy-sweet]

They also store some state related to the presentation of the canvas that is independent of the layers (i.e. grid, scale_bar, text_overlay, overlays). Would a space have its own instances of those? My guess is that a space might store some (e.g. the text overlay may only be relevant for that space), but not others (e.g. if you want a scale bar in one canvas, you probably want it in others).

[brisvag]

That's a good question, I didn't think about those... My gust reaction is yes, they should store that state; in fact, it might be as simple as doing (pseudocode):

viewer.spaces[0] = viewer.dict()
...
viewer.update(viewer.spaces[0])

I'm also starting to be convinced by the "states" name argument, since it's clearly more than just a "physical" space that we need to track.

And yes, I realize that at this point we are simply reproducing the full ViewerModel, which kinda weakens my argument against using "multiple viewers"... I still think it's better to not keep many ViewerModel objects (the you have to deal with connecting/disconnecting events, the slowness of EventedModel instantiation, ...); also, I still find the naming scheme of that one just too confusing.

[alisterburt]

you have to deal with connecting/disconnecting events, the slowness of EventedModel instantiation

Does having multiple Viewer objects mean this? How is it different from a space (~=ViewerModel) which also needs to update the GUI?

[brisvag]

They also store some state related to the presentation of the canvas that is independent of the layers (i.e. grid, scale_bar, text_overlay, overlays).

I added this to the API proposal, with some space left for discussion. I will update again in a follow up PR once people have given their opinion.

[andy-sweet]

Not sure what you mean by snapshot here. Wouldn't each space have its own dedicated instance of Camera and Dims? And then if we wanted to refer to Viewer.dims that would refer to dims of the current space?

[andy-sweet]

Sorry, I didn't read... these are the instances of the current space.

[brisvag]

To calrify: I'm saying that Space should hold the state of these objects, not the object themselves (i.e: Dims.dict()).

[brisvag]

Clarified in the text.

[andy-sweet]

Suggested change

	In general, the most intuitive approach would be to mirror the basic `Layer` api (i.e: with `name`, `metadata`, possibly `source`...).
	In general, the most intuitive approach would be to mirror the basic `Layer` API (i.e: with `name`, `metadata`, possibly `source`...).

[andy-sweet]

Not sure exactly what you mean by this sentence. I understand that a space has a name and some layers associated with it through the layerlist, but I don't understand what its metadata and source are and how they're connected to the layer's.

[brisvag]

metadata is always useful; if someone wants to mark each space with (for example) different experimental conditions, that should be possible.

source could be populated the same way as layers, in case (as mentioned in the Plugins section) a reader were to return spaces rather than just layers (if that part of the proposal gets accepted).

[brisvag]

Clarified in the text.

[andy-sweet]

Nit: some of your headings (like this one) are in Title Case whereas others like ## Multiple viewers are in Sentence case.

Would be good to be consistent throughout this doc. I have a vague memory that napari doc headings were supposed to be in Sentence case, which is also my preference, but I can't find that rule right now. The NAP template uses Title Case, so if that's true, we should probably update the template.

Apologies for this long nit. And apologies for the English language.

[brisvag]

Hehe, no proble, thanks for pointing out!

[andy-sweet]

It would be good to add what you think is out of scope for this NAP/project - at least I often find that useful to be keep the size of the work manageable.

In particular, I think it would be good to clarify if a layer can be in more than one space. If not, then you don't have to worry so much about the kind of issues discussed in #4682.

[alisterburt]

cc @LCObus potentially interesting UX questions around the proposed APIs here

[alisterburt]

had a productive discussion with @brisvag this morning - some notes!

In the below I'm going to call 'spaces' ViewerState
If ViewerStates live on the viewer and the viewer is the top level object this provides a nice simple API for users who are used to interacting with the viewer

viewer.active_state = viewer_states[-1]  # other APIs possible for state application

This use case is more clunky if spaces live at a higher level

app = napari.get_app()  # could also provide an accessor for viewer states directly
app.viewer_states[-1].apply(viewer)  # other APIs possible for state application

If ViewerStates live on the viewer then they will live in multiple places if we have multiple windows - I don't think I like this. Multiple viewers in one window also feels hard to support if we store states on the viewers... I need to clarify my thoughts and write things down I think

[tlambert03]

If I understand the NAP process, this looks good to me as a draft.

same here! definitely worth some discussion. Thanks for formalizing this @brisvag.

I'm still not convinced that we need a new concept/name for this, as opposed to better management of multiple viewers, but I do welcome the opportunity to think about it nicely laid out like this.

[Czaki]

One big worry (that may be addressed in following PR). What should happen if we have layer in multiple spaces and modify it. Did we should make a copy of it to keep all other states untouched. Or it is OK that all spaces will be impacted?

[brisvag]

I'm still not convinced that we need a new concept/name for this, as opposed to better management of multiple viewers, but I do welcome the opportunity to think about it nicely laid out like this.

I think we really need to clarify exactly what we mean by multiple viewers here. I think that working with multiple ViewerModels is problematic (having to connect/disconnect all the events, the slowness of instantiation of many when starting napari, the fact that some fields would probably actually clash (like theme, or cursor).

This is assuming that we want a separation (and not a 1to1 relationship) of Window and ViewerModel (which it seems we do).

If you agree with the above, then we need some way of encoding the state of these ViewerModels, but in a way that's not exactly a ViewerModel itself. Call it Space or Steve, but we definitely need something else :P

If I'm missing the point, It would be nice to see an API (pseudocode) example of how you imagine interacting with multiple viewers in a way that does not require adding a new abstraction, so we can add that to the Alternatives and make it easier to compare!

[brisvag]

One big worry (that may be addressed in following PR). What should happen if we have layer in multiple spaces and modify it. Did we should make a copy of it to keep all other states untouched. Or it is OK that all spaces will be impacted?

I think yes, all spaces should be affected. This NAP is not about separating info about rendering from data, so layers will still be a single source of truth for both. If you want different data, you should get a different layer. This can of course change in the future if/when we get the data and rendering separated.

One reason why I think you're worried about this, is because you're thinking of Spaces in terms of States (maybe an argument against the name), which suggests that the whole viewer state is preserved, as if it was a snapshot in time, data included. But this is not the intended purpose; we want to preserve a few rendering-related things (dims, camera, which layers are loaded, maybe little more), but no info about the layer state themselves, nor about the window.

[tlambert03]

yep yep, I agree more is needed.

having to connect/disconnect all the events

by the way, I think this is actually a pretty critical problem in the current design of napari. Though it seems laborious to have to connect/disconnect all of the events of a given model from a given view... I think that that, in fact, should be a mandatory method in any of our objects that make connections at all. We in general just go nuts connecting things, without keeping track of the connections that have been made. But we have to make it get better about making it clear how to connect/disconnect all the events. (This is part of why #3184 got so hard.) So, if any part of this is motivated by the current difficulty in connecting/disconnecting all the events, then I'd say that's also something that should be tackled first

[brisvag]

So, if any part of this is motivated by the current difficulty in connecting/disconnecting all the events, then I'd say that's also something that should be tackled first.

While it's an annoyance, wouldn't say this issue is a motivation for this NAP; I was referring to how expensive it is to hook up all the events, rather than the technical challenge itself. I've seen first-hand in #4597 that this can hit performance quite a lot. On the other hand, such slowdown might be less relevant in this case because we're not connecting the hundreds of events that get connected during validation in #4597.

[tlambert03]

We have three core dev approvals to add this in draft state. I'm going to merge this later today. This doesn't mean the NAP is accepted (see the NAP workflow if you want a reminder on the workflow), it just means enough people think it warrants more public discussion. @brisvag can gather the primary discussion points and update the NAP later