Copy units from layer to layer #7727
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7727 +/- ##
==========================================
- Coverage 92.98% 92.95% -0.03%
==========================================
Files 635 635
Lines 59857 59881 +24
==========================================
+ Hits 55656 55665 +9
- Misses 4201 4216 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This works really well, and I like it a lot.
I have:
- reviewed the code, and left one or two very minor comments,
- checked out the branch to play with the feature on my local computer. I've tried the new feature from both the GUI and the terminal, and it's really nice to use.
Thank you for making this happen @Czaki
| @@ -90,6 +93,21 @@ def test_copy_scale_to_clipboard(layer_list): | |||
| npt.assert_array_equal(layer_list['l2'].translate, (0, 0)) | |||
|
|
|||
|
|
|||
| @pytest.mark.usefixtures('qtbot') | |||
| def test_copy_units_to_clipboard(layer_list): | |||
There was a problem hiding this comment.
Could you write a short docstring describing what the test does?
Maybe something like:
Copy-paste spatial units from layer l1 to selected layer l2. Leaves unselected layer l3 unaffected.
Ideally we'd have something like this for all tests in this file, but I understand restricting this PR to just the part you are working on today.
There was a problem hiding this comment.
Why I suggest this: because developers unfamiliar with this section of the code have to do a bit of scrolling between the layerlist fixture object, and the test cases, before they can figure out what the test is supposed to check.
A short description would be especially helpful here because the tests are checking two things here (1) that the copy-paste works for selected layers, and (2) unselected layers are not affected.
There was a problem hiding this comment.
This looks good and I've pulled it down and played with it. One thing we may want to consider is copying from more ndim to fewer ndim works ok, but copying from fewer ndim to more ndim does not.
Why do we think it's ok to take the last n dimensions from a layer with more dimensions, and assign that to a layer with fewer dimensions, but not take all dimensions from a lower dimension layer and assign it into the last n dimensions of a higher dimension layer? I understand that we can't change the first dimensions (the ones that are unmatched), but it feels like if we're doing it one way, we should do it the other way too.
| @@ -104,7 +120,9 @@ def _paste_spatial_from_clipboard(ll: LayerList) -> None: | |||
| for layer in ll.selection: | |||
| for key in loaded: | |||
| loaded_attr_value = loaded[key] | |||
| if isinstance(loaded_attr_value, list): | |||
| if key == 'units': | |||
| loaded_attr_value = loaded_attr_value[-layer.ndim :] | |||
There was a problem hiding this comment.
why is it ok here to only get the last ndim values for units? Are we assuming that if the layer we copied from has more dimensions than the layer we're pasting to, the shared dimensions are the last ones? Just for my own understanding.
There was a problem hiding this comment.
Those are good questions, I didn't think to try mismatched dimensions. I'm also interested in the answer
There was a problem hiding this comment.
In general, if the target layer has fewer dimensions than the source one, we need to decide which use.
As we do not have named axis yet, I follow here the viewer semantic and get information from last axes.
Same as in line 137 of this file
(or on main)
Yes. This is intended to avoid guessing. And it is same for all spatial metadata, not only units. Under the hood it uses And the question is if only coping should works, or also assignment of attribute. |
@Czaki I guess I would be tempted to simply fail if the dimensions of the layers don't match, but maybe that's too stringent and limits usability of the feature...
Regardless of what we decide, I think this should only ever work when the user chooses to copy via the context menu. If you're programmatically assigning an attribute, it should do no guesswork whatsoever, and only work if the given attribute's dimensions are correct. |
I understand. But it is not trivial. And the current implementation in this PR is consistent with coping other spatial information. |
There was a problem hiding this comment.
Ok I think this is fine to go in then, just left some spell-check fixes but otherwise approving. Thanks for the additional info @Czaki
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
References and relevant issues
Description
Add copying units between layers using clipboard