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

[psobolewskiPhD]

Closes: #180
Closes: #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]

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 ⚠️
napari_animation/utils.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            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     

☔ View full report in Codecov by Sentry.
📢 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:

napari-animation/napari_animation/_tests/test_animation.py

Lines 9 to 18 in a9ca7a7

	CAPTURED_LAYER_ATTRIBUTES = [
	"name",
	"scale",
	"translate",
	"rotate",
	"shear",
	"opacity",
	"blending",
	"visible",
	]

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:

napari-animation/napari_animation/viewer_state.py

Lines 27 to 38 in a9ca7a7

	def from_viewer(cls, viewer: napari.viewer.Viewer):
	"""Create a ViewerState from a viewer instance."""
	layers = {
	layer.name: layer.as_layer_data_tuple()[1]
	for layer in viewer.layers
	}
	for layer_attributes in layers.values():
	layer_attributes.pop("metadata")
	layer_attributes.pop("properties", None)
	return cls(
	camera=viewer.camera.dict(), dims=viewer.dims.dict(), layers=layers
	)

But in tests only image layers are tested:

napari-animation/napari_animation/_tests/conftest.py

Lines 42 to 52 in a9ca7a7

	@pytest.fixture
	def image_animation(make_napari_viewer):
	viewer = make_napari_viewer()
	viewer.add_image(np.random.random((28, 28)))
	return Animation(viewer)
	@pytest.fixture
	def layer_state(image_animation: Animation):
	image_animation.capture_keyframe()
	return image_animation.key_frames[0].viewer_state.layers

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:
#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]

Should this test assert something? like that an exception is not thrown? I mean if there is an exception then it's a fail--like on main with this test--so maybe this is fine?

[brisvag]

I think it's just checking that there's no exception, yes. You could add an assert later that the viewer did indeed change, but maybe that's tested elsewhere?

[psobolewskiPhD]

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

[jni]

I'm confused about this — is this something users would need to do if trying to animate a Surface layer? That seems clunky. Basically, the things in the test should mirror what users would do, and if it's some weird workaround, then it might be good to e.g. push those tests to a different xfail test, so that it's obvious what needs to be fixed in the codebase going forward...

[psobolewskiPhD]

So the issue is, as Lorenzo noted, that these are enums flagged as immutable, so they arn't settable. So the layer state has them, but the viewer_state does not. The test compares the two sets of dicts, so I pop them from the layer state dict before the comparison check.
I don't think a user will ever run into this because they are immutable they can't changed during an animation anyways.

[psobolewskiPhD]

Ha! Turns out I had a horrible logic bug, see:
https://forum.image.sc/t/issue-saving-animations-in-napari-animation/88868/8?u=psobolewskiphd
And with it fixed this hack isn't needed. I was setting only the states that didn't change, which would be the immutable ones.
Now when we set the changed ones, everything should be peachy!

[jni]

Can we drop these lines now, for the same reason as the surface hack? Or maybe do something like:

original_layer_state = {
    k: v for k, v in layer_state.items()
    if k in CAPTURED_LAYER_ATTRIBUTES
}

or something like this? I don't like that this stuff is "hardcoded" in the test...

[jni]

(also, arn't is a typo. 😜)

[psobolewskiPhD]

Metadata isn't captured

napari-animation/napari_animation/viewer_state.py

Lines 17 to 19 in 35e5acc

	layers : dict
	A map of layer.name -> Dict[k, v] for layer attributes for each layer in the viewer
	(excluding metadata).

and it's popped in viewer_state

napari-animation/napari_animation/viewer_state.py

Line 34 in 35e5acc

layer_attributes.pop("metadata")

So I think that one has to be popped.
data doesn't get captured either:
https://github.com/napari/napari-animation/blob/35e5accac9c6994b9735bc7ecb1d679e2b70bdc5/napari_animation/viewer_state.py#L30C43-L31
Only the 2 element of the tuple is used.
While it could be cool if it worked, I'm not sure how keyframes and interpolation would work with data.

So the idea in the test would be to make a NEVER_CAPTURED_ATTR variable at the top of the file and put those there?

[brisvag]

While it could be cool if it worked, I'm not sure how keyframes and interpolation would work with data.

I a user wants to use napari-animation to change smoothly change the contents of data through out, there's a very good chance they actually need to add a dimension to Dims and use that instead :P

[psobolewskiPhD]

@jni does my latest commit
106365b
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! 😊