Add tests to cover slicing behavior when changing layers and data

[mstabrin]

References and relevant issues

Related to #6198 and #6199

Description

This adds tests to cover cases when either layers are added, removed, or their data changes in ways that should trigger data to be sliced and the canvas view to be updated. Some of these cases pass due to recent changes (e.g. #5522), but some fail due to remaining issues (e.g. #6198 and #6199).

[codecov]

Codecov Report

Merging #4819 (1f0934a) into main (70b8ea4) will increase coverage by 0.00%.
The diff coverage is 91.13%.

@@           Coverage Diff           @@
##             main    #4819   +/-   ##
=======================================
  Coverage   91.67%   91.67%           
=======================================
  Files         582      582           
  Lines       51073    51152   +79     
=======================================
+ Hits        46819    46894   +75     
- Misses       4254     4258    +4     

Files Changed	Coverage Δ
napari/components/_tests/test_add_layers.py	94.81% <91.13%> (-5.19%)	⬇️

... and 1 file with indirect coverage changes

[Czaki]

This is not a real problem, and it is not a proper solution. Accepting this will make adding a bunch of layers a quadratic operation, as adding a single layer will have linear complexity. The proper solution is to update layers when the range is changed.

Some signal is not emitted from the Dims object when, because of a change of range, the current slider position represents a different value in real-world coordinates.

[jni]

That's interesting, thanks @mstabrin for the report! I think the fix is not the right fix here though. (Again... 😅 I swear I am not trying to be annoying! 😂)

The issue is that we update all layers based on the viewer.dims.current_step changing, and the current_step hasn't changed here, even though the meaning of current_step has changed. So I think the solution is to update all layers when the viewer.dims.point has changed. Currently there is no event mapped to that so it should be added...

The reason I think this is incorrect is that a very very common use case is to add layers that occupy exactly the same space, and this fix would now make that use case much slower.

[mstabrin]

@jni I totally agree and I think it is an inefficient solution.

At first I tried to identify if the lower range changed, but could not find a way so I presented a solution with minimum changes required :)

I will look into a points event, using this opportunity to learn more about events^^ I will use this PR here so jo need to close it :)

[mstabrin]

Sooo, after some research on how the event system works I added a new event for point and connected it to the update_layer.
It appears that it is only updating all layers now when the point property changes.

[Czaki]

This solution looks working, but dims.points are updated on the moving dims slider. And, as you show in the first post, after moving the slider, the layers are updated. So with this change we will have the double update of layers.

But I'm not familiar with this part of the napari codebase. Calling @andy-sweet, who may better know what needs to be done.

[jni]

Ok I'm confused... It looks from the diff that there was already a point event on dims? But I looked at that specifically when I responded yesterday and didn't find it:

In [5]: viewer = napari.Viewer()

In [6]: viewer.dims.events.emitters
Out[6]:
{'ndim': <napari.utils.events.event.EventEmitter at 0x15022ecb0>,
 'ndisplay': <napari.utils.events.event.EventEmitter at 0x15022d180>,
 'last_used': <napari.utils.events.event.EventEmitter at 0x15022d5a0>,
 'range': <napari.utils.events.event.EventEmitter at 0x15022ec20>,
 'current_step': <napari.utils.events.event.EventEmitter at 0x15022d6c0>,
 'order': <napari.utils.events.event.EventEmitter at 0x15022c400>,
 'axis_labels': <napari.utils.events.event.EventEmitter at 0x15022dde0>}

At any rate, the test failures are real... Not sure immediately what the fix is.

But @Czaki @mstabrin, I think the fix to the double update is simply to remove the current_step connection from the line above.

[Czaki]

How could I miss the current_steep connection line above...

Yes. It is a proper solution from my point of view.

[andy-sweet]

Ok I'm confused... It looks from the diff that there was already a point event on dims? But I looked at that specifically when I responded yesterday and didn't find it:

There is a little extra magic in EventedModel that adds an event to any property with a setter. So the event is not on main, but it is after the current changes in this PR.

[andy-sweet]

This is not a real problem, and it is not a proper solution.

Seems like a real problem to me - the view should update when adding a new layer. But I don't understand the current solution at all.

I am not sure how to test this, I will look into it later.

I think the following test should work. It tests some private state, which is not ideal, but seems fine to me. Also no need for the out_of_slice_display to reproduce the core bug here.

def test_add_points_layer_with_different_range_updates_all_slices():
    """See https://github.com/napari/napari/pull/4819"""
    viewer = ViewerModel()
    point = viewer.add_points([[10, 5, 5]])

    other_point = viewer.add_points([[8, 1, 1]])

    np.testing.assert_array_equal(point._indices_view, [])
    np.testing.assert_array_equal(other_point._indices_view, [0])

I think the solutions I would go to have already been tried/mentioned.

1. Connect the range event to _update_layers because point is dependent on both current_step and range.
2. Update all the layers whenever any layer is added.

The quadratic performance of adding lots of layers with the same extent is concerning, but I believe the early return condition in Layer._slice_dims should prevent us from actually doing much work in that case.

[Czaki]

Seems like a real problem to me - the view should update when adding a new layer. But I don't understand the current solution at all.

the current problem is that adding a new layer does not cause recalculation of the slider position. So 0 in the current steep happens to be 0 in a new steep, but calculation from value to world space changes. (so current_step does not change but its implementation change).

[andy-sweet]

the current problem is that adding a new layer does not cause recalculation of the slider position. So 0 in the current steep happens to be 0 in a new steep, but calculation from value to world space changes. (so current_step does not change but its implementation change).

I see. Thanks for a different perspective. I guess the implied desired behavior is that we should try to maintain the value of point (which effectively defines the slicing plane in world coordinates) when a new layer is added?

In general that won't be possible because the slider has integer positions. But we could snap to the closest slider position. I think that behavior is effectively defined by Dims.set_point already. So maybe there's a solution where we call Dims.set_point after the layer is added with the value of Dims.point before the layer is added?

[andy-sweet]

@mstabrin : I finally picked this up again. And I have mostly good news :)

I merged main into this and mostly accepted changes from main. See the diff with main on that branch for more details.

The good news is that test_add_points_layer_with_different_range_updates_all_slices passes without any changes needed to core napari. So I think this means that the recent changes from @brisvag fixed those issues.

The bad news is that the other two tests (test_last_point_is_visible_in_viewport, test_dimension_change_is_visible_in_viewport) still fail. I think the core reason that happens is because while Dims.range changes correctly, we only respond to changes to Dims.point. I think point does change, but its event is never emitted because it happens in the root validator of Dims. I don't remember if that was the same reason when this PR was written.

My suggestion is to merge in the passing test (test_add_points_layer_with_different_range_updates_all_slices), which runs quickly and should prevent future regressions. Then create an issue (or maybe two) to cover the other failing tests, which we can use as reproducers. I'm happy to handle both of these. What do you think?

[mstabrin]

@andy-sweet: Glad to hear that the first test is passing with the recent changes! :)
I think it would be a good idea to have the passing test merged.
Maybe the other two could be merged as well marked with xfail like you suggested in your previous comment?
So the new issues could directly link to the already failing existing test?

[andy-sweet]

I think it would be a good idea to have the passing test merged.
Maybe the other two could be merged as well marked with xfail like you suggested in your previous comment?
So the new issues could directly link to the already failing existing test?

We don't have many xfails in napari, but I don't think we're against them either, so that sounds good.

Do you mind if I push directly to this branch? Or would you prefer if I pushed to my own and made a PR to yours?

[mstabrin]

I don't mind :)
You can merge directly!

[andy-sweet]

I don't mind :) You can merge directly!

Done! The two tests are marked with xfail where the reason links to the bugs. I think the bugs are slightly different, so I created separate ones, but I think they may share a solution.

[andy-sweet]

@brisvag : I added you as a reviewer because I think your recent work on dims (i.e. making it fundamentally based on point rather than current_step) fixed some of the issues here. I think the remaining failing tests (marked with xfail) could be fixed relatively easy as a small extension of that work (e.g. by ensuring a change in point always emits an event).

[brisvag]

I'm not sure I follow the difference in the succeeding and failing tests... Are all the asserts in the xfail tests supposed to fail?

I doubt the event is not happening, that would be a pretty huge issue deep down in the evented model which would affect the whole codebase. I think the source might be that we connect to current_step instead of point. This is not a problem for normal use, cause we never set the point to in-between steps unless we do it manually... which we may be doing here. I fixed this connection in #5697, but you could try adding it here.

The line is:

napari/napari/components/viewer_model.py

Line 259 in 55b9a97

self.dims.events.current_step.connect(self._update_layers)

There's a few other places where we connect to current_step which should be changed (except for the qt module).

[andy-sweet]

I'm not sure I follow the difference in the succeeding and failing tests... Are all the asserts in the xfail tests supposed to fail?

No, only one fails currently in each test. I think that's clarified in the reproducers in the bug write ups. Can also add comments in this PR if desired.

I fixed this connection in #5697, but you could try adding it here.

Oh yeah, I forgot that change is not in yet!

I doubt the event is not happening, that would be a pretty huge issue deep down in the evented model which would affect the whole codebase.

Even if that change comes in, I think there's still an issue (I tried the suggested change out on this branch and the new tests still fail). Here's a reproducer that shows the issue when changing Dims.range causes Dims.point to change but doesn't emit its event:

from napari.components.dims import Dims
d = Dims()
d.events.range.connect(lambda: print('range change'))
d.events.point.connect(lambda: print('point change'))
print(d.range)  # -> (RangeTuple(start=0.0, stop=2.0, step=1.0), RangeTuple(start=0.0, stop=2.0, step=1.0))
print(d.point)  # -> (0, 0)
d.range = ((1, 2, 1), (1, 2, 1))
# -> 'range change'
# but not -> 'point change'
print(d.range)  # -> (RangeTuple(start=1.0, stop=2.0, step=1.0), RangeTuple(start=1.0, stop=2.0, step=1.0))
print(d.point)  # -> (1, 1)

I suspect the issue is that the change to point happens in the root validator and that may bypass the setter, which is why our event never gets emitted. We could also connect to the range event for slicing, but that might cause more slicing than desired in some cases (at least for sync slicing). Happy to write up a general issue for this since I think it applies to any values updated in the root validator.

[brisvag]

I suspect the issue is that the change to point happens in the root validator and that may bypass the setter, which is why our event never gets emitted. We could also connect to the range event for slicing, but that might cause more slicing than desired in some cases (at least for sync slicing). Happy to write up a general issue for this since I think it applies to any values updated in the root validator.

I was about to say that this shouldn't be an issue, but I think you're right actually... Cause when we check for equality, we only check for dependency of properties, and we don't have any concept of dependencies of fields... @Czaki, do you think we could simply manually add dependencies even if they're not properties, and get proper behaviour from our current logic?

The brute force alternative is to always check equality for all values (maybe special-casing Dims) instead of just dependent ones... More expensive, but guarantees events are fired.

[andy-sweet]

@mstabrin : I changed the title and description dramatically based on the new changes in the PR. Please let me know if that's not OK and/or feel free to make edits.

[andy-sweet]

I was about to say that this shouldn't be an issue, but I think you're right actually... Cause when we check for equality, we only check for dependency of properties, and we don't have any concept of dependencies of fields... @Czaki, do you think we could simply manually add dependencies even if they're not properties, and get proper behaviour from our current logic?

I think a similar issue would arise for psygnal's EventedModel, though I haven't actually tried that yet. What do you think about writing an issue there and seeing if we can port a similar to solution to our EventedModel?

[brisvag]

We have a few other changes to our evented model which may not be ported there yet (like the most recent changes from @Czaki). We should definitely try to port those these and also solve this, and then use psygnal as a dependency :P

[andy-sweet]

I went ahead and merged this since the test cases are valuable and everything is green. We have the two issues to follow up on, which might share a more common general fix around EventedModel or not.

Thanks again for your help and patience here @mstabrin . Hopefully we can get the remaining issues fixed soon.