[Bugfix] For `layer_attributes` that are dict (like `color`) do key/value comparisons

[psobolewskiPhD]

Closes: https://github.com/napari/napari-animation/issues/180 Closes: https://github.com/napari/napari-animation/issues/184

This adds a new function to utils.py for carrying out the comparison of layer_attribute. In particular if an attribute is a dict then the function recurses using key/value pairs. This is needed for example for Labels layer color which is a dict of np.array. But other layers also have nested dicts (e.g. Surfaces)

Additionally, I implement two tests that use all napari layers. The first compares attributes between viewer_state and the actual layer and the second checks whether the animation has frames. These tests fail in main but pass with the fix mentioned above.

[codecov[bot]]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (a9ca7a7) 86.25% compared to head (f5130d8) 86.38%. Report is 3 commits behind head on main.

Files	Patch %	Lines
napari_animation/viewer_state.py	33.33%	4 Missing :warning:
napari_animation/utils.py	83.33%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #181 +/- ## ========================================== + Coverage 86.25% 86.38% +0.13% ========================================== Files 26 26 Lines 1011 1043 +32 ========================================== + Hits 872 901 +29 - Misses 139 142 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jni]

@psobolewskiPhD this seems ok but I don't fully understand the issue. Perhaps there is a test that you can make for this...?

[psobolewskiPhD]

@jni yeah a test would probably be good. It doesn't look like different layers are tested at all and they do have different attributes... Here's a simple breakdown of the issue: Labels layer has a color attribute:

In [12]: type(viewer.layers[0].color)
Out[12]: dict

which is, for 1 label, for example:

In [10]: viewer.layers[0].color
Out[10]: 
{0: array([0., 0., 0., 0.], dtype=float32),
 None: array([0., 0., 0., 1.], dtype=float32)}

So it's a dict with an array inside. Trying to compare such dict fails:

In [2]: import numpy as np

In [3]: a =  {0: np.array([1, 1, 1]), None: np.array([0, 0, 0])}

In [4]: b = {0: np.array([1, 1, 1]), 2: np.array([0, 0, 0])}

In [5]: np.array_equal(a, b)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[5], line 1
----> 1 np.array_equal(a, b)

File ~/micromamba/envs/napari-418/lib/python3.10/site-packages/numpy/core/numeric.py:2439, in array_equal(a1, a2, equal_nan)
   2437     return False
   2438 if not equal_nan:
-> 2439     return bool(asarray(a1 == a2).all())
   2440 # Handling NaN values if equal_nan is True
   2441 a1nan, a2nan = isnan(a1), isnan(a2)

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

[psobolewskiPhD]

Actually, I wonder if the test issue is here: https://github.com/napari/napari-animation/blob/a9ca7a7a96e5b281bf80815aaded8a6ce0b72872/napari_animation/_tests/test_animation.py#L9-L18 color is not one of the attributes

[jni]

Do you want to try adding it and see what happens to the CI? 😅

[psobolewskiPhD]

So having looked at it a bit more, it's a bit trickier. Those attributes are set here: https://github.com/napari/napari-animation/blob/a9ca7a7a96e5b281bf80815aaded8a6ce0b72872/napari_animation/viewer_state.py#L27-L38 But in tests only image layers are tested: https://github.com/napari/napari-animation/blob/a9ca7a7a96e5b281bf80815aaded8a6ce0b72872/napari_animation/_tests/conftest.py#L42-L52

So handling of attributes like color for labels is never checked, same with Points and I assume other layer types.

[psobolewskiPhD]

So an alternate way to solve this is to add color to the list of attributes not tracked, like @alisterburt did here: https://github.com/napari/napari-animation/pull/141 In fact, it may be more sane to make a list of specifically tracked attributes instead?

Eitherway, to support other layer types, tests should test them.

[imagesc-bot]

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/issue-saving-animations-in-napari-animation/88868/2

[psobolewskiPhD]

@brisvag Took me a while to get back to this, but per our conversations at the hackathon, I added a recursive function to compare attributes: check_layer_attribute_changed in napari_animation/utils.py It's somewhat naive and only recurses dicts, but I'm pretty sure that covers everything we need? I did need to pop some more dict entries because they arn't settable so setattr failed. With this change all animations work for me locally.

I then added tests, using napari fixtures to test all layers--I figured there was no need to reinvent the wheel when we have nice fixtures for making a buncha layers with data. Previously only image layer was tested. I didn't remove the old tests just changed the list of attributes at the top to clearly state it's image layer only. Now the new test for actually animating every layer type passes with this PR but fails with main for everything but Image. I then added an all_layer_types variant of the test that checks the viewer_state attributes. It passes with one exception: the Surface layer (which does animate fine locally)

The fail is that the viewer.layers[0]._get_state()['normals']['face']['mode'] isn't set and stored in the viewer_state so the viewer_state has 1 less item in that 'face' dict.

Anyone have any insight into that? is it because it's an ENUM: https://github.com/napari/napari/blob/46a3683dc7d7857d5b269a321649847fe864d0fd/napari/layers/surface/normals.py#L8-L10

[brisvag]

It's somewhat naive and only recurses dicts, but I'm pretty sure that covers everything we need?

Not 100% sure, but yes, I think evented models shoudl always end up giving nested dicts with actual objects as leaves.

The fail is that the viewer.layers[0]._get_state()['normals']['face']['mode'] isn't set and stored in the viewer_state so the viewer_state has 1 less item in that 'face' dict.

Anyone have any insight into that? is it because it's an ENUM: napari/napari@46a3683/napari/layers/surface/normals.py#L8-L10

Mhm... not sure I understand the error. But surely we have many other enums that are not failing?

[psobolewskiPhD]

yeah it's odd. I tested locally by making a random Surface layer and then an animation and indeed that ['normals']['face'] does not have Mode set. i've never really used surfaces so not quite sure what it does. I'll poke around a bit more. I guess if I don't figure it out could skip that one attribute?

[brisvag]

Could it just be that it has allow_mutation=False and we don't avoid trying to set that?

[psobolewskiPhD]

Yup, that would be it I think, makes them unsettable. So I guess skipping that attribute would be correct?

[psobolewskiPhD]

JNI approved but I've changed a lot here, so lets consider that a stale review -- I can't dismiss it.

[psobolewskiPhD]

@jni does my latest commit https://github.com/napari/napari-animation/pull/181/commits/106365bfa623d37db18221bf08303ae1dd104f26 do what you indicated in your review comment?

Would be nice to get this merged and then see about a release--it unblocks the docs effort too.

[jni]

Well enough! Feel free to merge here! 😊