Add mechanism for inheritance of affine, rotate, scale, shear, translate
#6827
Conversation
Czaki
changed the title
affine, rotate, scale', shear, translate`affine, rotate, scale, shear, translate
|
Could you explain a bit the background of this feature, what problem it tries to solve? |
If one layer does not have some parameters set, then try to guess them to keep layers in same coordinate space. For example:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6827 +/- ##
==========================================
+ Coverage 92.45% 92.49% +0.04%
==========================================
Files 617 618 +1
Lines 55161 55484 +323
==========================================
+ Hits 50998 51319 +321
- Misses 4163 4165 +2 ☔ View full report in Codecov by Sentry. |
| @@ -44,7 +44,7 @@ def test_scaled_and_translated_images(): | |||
| viewer = ViewerModel() | |||
| np.random.seed(0) | |||
| data = np.random.random((10, 10, 10)) | |||
| viewer.add_image(data) | |||
There was a problem hiding this comment.
These changes worry me a bit; does this mean that existing layers will have their transforms changed by newly added layers? I feel like it should only work the other way around.
There was a problem hiding this comment.
Yes. Currently, it works both way. I try to get feedback about this for 3 weeks. I'm not sure which option is better.
There was a problem hiding this comment.
I think once a layer's in the viewer, I'd be confused if it suddenly changed. The other way around is more likely to be the desired behaviour, even though mostly from pressing the new layer buttons in the gui. Ther are other cases (like the CSV example above) where this would be nice when coming from readers, but honestly I'd prefer the expplicit action from #6864.
There was a problem hiding this comment.
In contradiction, when you have bidirectional inheritance data presentation do not depend on load order. And you could assume that neither non of layer has set given property or all layers has given property.
So user will not ask, why loading image before annotation is important for proper presentation.
Once there is the ability to copy property from gui it is not so important.
There was a problem hiding this comment.
Fair point... Given this, I think even more I'd prefer the explicit copy.
There was a problem hiding this comment.
But what with axis_labels and units (planned for #6780) it is impossible to do it without such sort of inheritance.
Or maybe I should limit inheritance only to scale and assume that the rest should be done via right click?
There was a problem hiding this comment.
I try to get feedback about this for 3 weeks.
At the community meeting 3 weeks ago I was also quite apprehensive about modifying existing layers. As we discussed then, I do agree with the converse concern, that it's somewhat icky if layer-addition operations are not commutative. On the other, it's easily comprehensible (new layers inherit from existing layers), and it gives users an easy pathway to getting either result — all layers get the metadata, or only the layers with explicit metadata get metadata.
Given napari's GUI-only use cases, I can relax my general "let's not do magic" policy to "all magic is dark magic". Layer metadata inheritance is magic, so some ickyness is inevitable. I think my and Lorenzo's instincts suggest that the non-commutative ickyness is preferable to the existing-layer-overwriting ickyness.
It's also a bit of a red flag when many existing tests need to be modified to work correctly. By removing the backwards effect, we can reduce the number of changes we need to make in this PR.
I also might suggest that we should only do magic in the GUI and not in the API. But I'm less sure about this.
There was a problem hiding this comment.
I also might suggest that we should only do magic in the GUI and not in the API. But I'm less sure about this.
I do not know how to distinguish it, especially in context of plugins.
Given napari's GUI-only use cases, I can relax my general "let's not do magic" policy to "all magic is dark magic". Layer metadata inheritance is magic, so some ickyness is inevitable. I think my and Lorenzo's instincts suggest that the non-commutative ickyness is preferable to the existing-layer-overwriting ickyness.
What with units and axes labels? There need to be bidirectional inheritance?
As I think now, I think that scale, units and axis labels should be bidirectional. Translate, shear single directional and affine only using manual coping. What did you think?
There was a problem hiding this comment.
I do not know how to distinguish it, especially in context of plugins.
Treat viewer.open and viewer._add_from_layer_data_tuple (I forget what it's called) differently from viewer.add_*? Maybe better is to distinguish Actions from API calls? Maybe just Qt events (drop, file -> open, etc)?
Those are just vague ideas — I did say I was less sure. 😅 Happy to skip this.
What with units and axes labels? There need to be bidirectional inheritance?
Does there? Once we start setting the viewer/canvas axes from the union of the layer data, maybe. But again, maybe not? If we provide a way for the user to get the result (in fact two: add the layers in a different order, or copy the metadata manually), then they can fix their issue. The converse is not true in the case where we incorrectly inherit.
As I think now, I think that scale, units and axis labels should be bidirectional. Translate, shear single directional and affine only using manual coping. What did you think?
It sounds messy and complicated compared to unidirectional for everything. I'm getting a headache re logic flow just thinking about it. 😂 And I don't like the inconsistency between attributes, especially between scale and translate.
Even manual copy for everything is starting to sound nice, just from the perspective of code maintenance.
There was a problem hiding this comment.
Treat
viewer.openandviewer._add_from_layer_data_tuple(I forget what it's called) differently fromviewer.add_*? Maybe better is to distinguish Actions from API calls? Maybe just Qt events (drop, file -> open, etc)?
viewer.open and viewer._add_from_layer_data_tuple are using viewer.add_* internally.
Limiting to Qt events is impossible, because of functional plugins.
Does there? Once we start setting the viewer/canvas axes from the union of the layer data, maybe. But again, maybe not? If we provide a way for the user to get the result (in fact two: add the layers in a different order, or copy the metadata manually), then they can fix their issue. The converse is not true in the case where we incorrectly inherit.
So what do if one layer have units, and the second do not have, but they have the same axes names? By axes names they should be presented in the same coordinate space, but it is impossible for non-consistent units.
Even manual copy for everything is starting to sound nice, just from the perspective of code maintenance.
And from the perspective of user, instead of just load layer, or execute plugin, then there is a need to all time use right click button until all plugin authors update their plugin.
I haven't taken a proper look at this PR, but as a minor bonus, this fixes the almost 3-year old #3058 In the discussion there, we suggested removing the lookup by name feature entirely, but none of us felt too strongly. Given that supporting setting by name is easy, I think the addition here is also fine. Though I'd maybe expect the functionality to be in |
References and relevant issues
extracted from #6780
Description
This PR infroduces:
SampleLayer- simple layer to simplify and speedup testing that does not depend on given layer type. It is minimum class inheriting fromLayerparameters_with_default_valuesproperty toLayerandLayerListto hold information which properties have default values.affine,rotate,scale,translate(I need to dive deeper in shear).TransformChainby name, not index (I may extract it to separate PR).This PR adds a basic test. It will be improved If the whole shape looks good.