Codecov Report

Merging #2855 (35f642f) into master (472ea41) will increase coverage by 0.02%.
The diff coverage is 90.56%.

@@            Coverage Diff             @@
##           master    #2855      +/-   ##
==========================================
+ Coverage   82.90%   82.92%   +0.02%     
==========================================
  Files         500      500              
  Lines       41977    41992      +15     
==========================================
+ Hits        34801    34822      +21     
+ Misses       7176     7170       -6     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 472ea41...35f642f. Read the comment docs.

Awesome PR! Posting some suggestions from the relevant Zulip discussion and adding some comments about the PR itself:

1. Rather than layer.data2world_transform, which is completely inconsistent with the pre-existing layer.world_to_data, we should either add layer.data_to_world or (probably my preference) making the layer._transforms TransformChain publicly accessible (either as read-only or as a copy) as layer.transforms, and adding attribute access to that object, ie layer.transforms.world_to_data, layer.transforms.data_to_world. I don't know whether we want to restrict access to just those two, but if so, we could potentially simplify as layer.transforms.to_world and layer.transforms.to_data.
   Having said all of the above, we could leave that tweak to a future PR and just expose data_to_world here to be consistent with world_to_data.
2. world_to_world is certainly weird and suggests that we need to rethink that nomenclature. (What is the inverse of world_to_world called? 😂) There should only be one world, and the pre-transformation coordinates should have a different name... acquisition? sensor? preregistered? We should keep in mind this comment from the NGFF transformation discussions:

   I therefore strongly believe that named axes with references to physical spaces and units should be used for the target space of a transformation, not the data/ source space. The data has axes in null or data space, it's default unit is "px".

If we can address those two points (again, data_to_world would be sufficient for point 1 for this PR, we can follow up later), 💯 from me! Love this, as mentioned in Zulip, I have immediate use for this stuff. 😃

Thanks for the review @jni and @sofroniewn!

@jni: As for (1), I actually didn't notice Layer::world_to_data until you mentioned it and agreed that consistency with that is desirable. That's a little tricky for a couple of reasons:

A. world_to_data is a function with a coordinates argument that returns a tuple whereas data_to_world needs to return an Affine transform based on other current usage.
B. The inverse transform defined from the chain in the world_to_data includes the world2grid transform whereas the data_to_world transform defined in this PR does not. I think that's because world_to_data mostly seems to be about going from OpenGL's world coordinates (yet another world :D) to a layer's data coordinates.

I think we might be able to resolve (A) by making world_to_data be a Transform property without breaking any usage because we have Transform::__call__ that will handle calls like Layer::world_to_data(coords). But we need to be careful about continuing to handle padding/truncating coordinate dimensions appropriately.

The simplest resolution for (B) is to include world2grid in the definition of the data_to_world transform. In our current usage, I think world2grid is always a translation and data_to_world doesn't seem to care about the translation component of the transform, but it would be good for someone else to check that.

Alternatively, to solve both these issues we could define Layer::_data_to_world as a hidden method that is a little inconsistent with world_to_data before we decide how best to expose things publicly.

As for (2), agreed that world2world is bad. Other ideas were worldaffine or worldextra, but I didn't really like those either. I'm OK with introducing another intermediate space like acquisition (or maybe physical based on that quote from the NGFF discussions) so that we have something like data2acq -> acq2world -> world2grid and that's probably the simplest thing I can do in this PR. However, those names aren't publicly exposed and I'd like to keep it that way (for now at least).

I think we might be able to resolve (A) by making world_to_data be a Transform property without breaking any usage because we have Transform::call that will handle calls like Layer::world_to_data(coords). But we need to be careful about continuing to handle padding/truncating coordinate dimensions appropriately.

👍 A+ to this. Actually world_to_data did trip me up yesterday/today working on jni/zarpaint#9, where I needed to transform points to coordinates in another layer's data, and I ended up using the transforms directly. So I think it's desirable to (a) ensure that Transforms handle dimension broadcasting correctly, and (b) to replace world_to_data with an actual transform.

The simplest resolution for (B) is to include world2grid in the definition of the data_to_world transform. In our current usage, I think world2grid is always a translation and data_to_world doesn't seem to care about the translation component of the transform, but it would be good for someone else to check that.

Can you elaborate on this? What do you mean that it doesn't "care"? If I give you some coordinates and run them through the transform, they definitely will come out different when there's a translation vs not, right???

Thinking about this, I don't think I like the idea of including world2grid. As seen in my PR above, at least one use case for exposing these things is to be able to move coordinates between layers — which will be impossible if the grid is included, since by definition the grid separates the layers' data in space.

So, I think we should:

• deprecate world_to_data and rename it as grid_to_data.
• add a public .transforms attribute. This can just be the current ._transforms for now, or we can curate it. Thoughts from @sofroniewn would be welcome.
• I wonder if we should update the API. Instead of src2dst, we could do layer.transforms['src', 'dst'], and grab the relevant transform chain there. Actually I just saw that `layer.transforms['src':'dst'] is valid Python, which is awesome. How do people like that as an API?

Regarding the intermediate space, I'm not into abbreviations — it's why we call things hook_implementation rather than Pluggy's native hookimpl, which I found inscrutable. So I'd much prefer acquisition rather than acq.

Thanks for the comments @jni !

Can you elaborate on this? What do you mean that it doesn't "care"? If I give you some coordinates and run them through the transform, they definitely will come out different when there's a translation vs not, right???

By it doesn't care, I meant that after my changes here all current usages of data_to_world outside of the Layer class would behave the same assuming that the world2grid transform is always a translation (i.e. those usages are translation invariant). However, you're right that the presence of an extra translation will matter for some of the usage of data_to_world within Layer and it will obviously affect new usage too. So agreed that we probably shouldn't include world2grid.

I think I'm good with your proposals, my main two issues being the following.

1. The world2grid transform feels like a temporary workaround to do side-by-side views and similar while we only have one canvas, so exposing it now may mean that we'll deprecate it later.
2. While TransformChain is neat, it also feels a bit over engineered based on its current usage. By exposing it now, we might be stuck with it for a while.

As I'm still new here, it's hard for me to put these issues in context, so additional comments would be welcome. My sense is that it might just be worth exposing them, seeing how they work out as we and others use them, and deprecate/refactor if needed later.

The world2grid transform feels like a temporary workaround to do side-by-side views and similar while we only have one canvas, so exposing it now may mean that we'll deprecate it later.
While TransformChain is neat, it also feels a bit over engineered based on its current usage. By exposing it now, we might be stuck with it for a while.

I agreed with both these points. See #1690 for a long time issue of getting the grid information off the layers - it really doesn't belong there are all, so deprecating something to add something in with grid seems like I step in the wrong direction. I think i'd prefer to get this in without exposing new things publicly as much as possible, and then as we continue our refactoring

Alternatively, to solve both these issues we could define Layer::_data_to_world as a hidden method that is a little inconsistent with world_to_data before we decide how best to expose things publicly.

I think this might just be easiest and would let us merge this PR and defer this until after #1690 and other improvements are made

Just one minor comment:

While TransformChain is neat, it also feels a bit over engineered based on its current usage. By exposing it now, we might be stuck with it for a while.

I don't know which bit you think is overengineered, certainly this could be true of several "clever" parts of it, but I don't think overall it is overengineered. The transformation chain is a fundamental part of rendering the data and if anything, we will need to get even more sophisticated if we want napari to e.g. be able to display geographic data using nonlinear projections. Which we do. 😜 Not least because we will also want to be able to display e.g. nonlinear distortions in ssTEM slice data. Once you start mixing that with e.g. correlated light data in the same space, you really need the transform chain, a very clear definition of your spaces, and both forward and inverse transforms.

But other than that yes, I agree that the above discussion does not fall under "minimal changes to support affine composition" 😂 and am happy to keep things very simple and private as a start! Just didn't want to codify 'world to world' or inconsistent naming. 🙏

How about we make _data_to_world private and go with 'data_to_physical' and 'physical_to_world' for the two transforms. Both of these are private so we can change them in the future. We can then merge this PR and continue with improvements in followups.

Thanks @jni and @sofroniewn for the comments again. I followed up by making _data_to_world private and changing the internal names of the transform chain (added data2physical and physical2world to stay consistent with the current internal x2y naming).

I left world_to_data alone for now. Its definition is inconsistent with its name (because it includes the world2grid transform), and thus also inconsistent with _data_to_world (which does not include world2grid), but _data_to_world is private at the moment, so that inconsistency isn't public. I considered deprecating world_to_data and adding _world_to_grid to resolve those inconsistencies, but it seemed like more changes than needed here.

I don't know which bit you think is overengineered, certainly this could be true of several "clever" parts of it, but I don't think overall it is overengineered.

I meant that its over engineered with respect to its current internal usage within napari, but I can see how it could be useful externally when trying to implement things like a non-linear transform on top. Given the conversation here and in PR 2793, it seems like the interface still needs a bit more thought and I'm happy to work on that over the next few weeks.