Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 23 commits into from
Dec 14, 2023
Merged

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

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
05122db
do key/value comparisons for dict
psobolewskiPhD Jul 19, 2023
66ef255
add test for all layers
psobolewskiPhD Nov 19, 2023
d273e69
formatting
psobolewskiPhD Nov 19, 2023
6f812d6
use a recursive check for changes
psobolewskiPhD Nov 19, 2023
c295663
Skip attributes that can't be set
psobolewskiPhD Nov 19, 2023
ff2db17
fix double imports :eyes:
psobolewskiPhD Nov 19, 2023
46adce5
flake8
psobolewskiPhD Nov 19, 2023
96299e8
don't write a movie
psobolewskiPhD Nov 19, 2023
58b1dba
add color attr to tests
psobolewskiPhD Nov 19, 2023
2411da0
skip vscode
psobolewskiPhD Nov 20, 2023
01761f8
test attributes for all layer types
psobolewskiPhD Nov 23, 2023
470c685
fix fixture
psobolewskiPhD Nov 23, 2023
24b54a8
skip Surface face and vertex modes that don't allow mutation
psobolewskiPhD Nov 23, 2023
ff31cf6
Check if surface before popping modes
psobolewskiPhD Nov 23, 2023
c21c760
simplify per @brisvag
psobolewskiPhD Nov 25, 2023
384e2a3
ensure dict not passed to np.array_equal
psobolewskiPhD Nov 25, 2023
a183ce8
Use try-except to handle not settable attr
psobolewskiPhD Nov 25, 2023
4df23b5
Partial revert: check for Surface mode
psobolewskiPhD Nov 25, 2023
e891efe
fix logic to actuall set changed attr
psobolewskiPhD Nov 26, 2023
86acb74
Fix tests, drop Surface hack
psobolewskiPhD Nov 26, 2023
3e3da4f
logic tweak from @brisvag
psobolewskiPhD Nov 28, 2023
106365b
make an exclude list for not stored layer attr
psobolewskiPhD Nov 28, 2023
f5130d8
Update napari_animation/_tests/test_animation.py
psobolewskiPhD Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,6 @@ target/

# written by setuptools_scm
*/_version.py

# vscode
.vscode
47 changes: 45 additions & 2 deletions napari_animation/_tests/test_animation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,16 @@

import numpy as np
import pytest
from napari._tests.utils import (
add_layer_by_type,
assert_layer_state_equal,
layer_test_data,
)

from napari_animation import Animation, ViewerState
from napari_animation.utils import make_thumbnail

CAPTURED_LAYER_ATTRIBUTES = [
CAPTURED_IMAGE_LAYER_ATTRIBUTES = [
"name",
"scale",
"translate",
Expand Down Expand Up @@ -125,7 +130,7 @@ def test_animation_file_metadata(animation_with_key_frames, tmp_path, ext):
assert b"https://napari.org" in content


@pytest.mark.parametrize("attribute", CAPTURED_LAYER_ATTRIBUTES)
@pytest.mark.parametrize("attribute", CAPTURED_IMAGE_LAYER_ATTRIBUTES)
def test_layer_attribute_capture(layer_state, attribute):
"""Test that 'attribute' is captured in the layer state dictionary"""
for layer_state_dict in layer_state.values():
Expand All @@ -139,3 +144,41 @@ def test_end_state_reached(image_animation):
image_animation.capture_keyframe(steps=2)
last_state = image_animation._frames[-1]
assert last_state == image_animation.key_frames[-1].viewer_state


@pytest.mark.parametrize("layer_class, data, ndim", layer_test_data)
def test_attributes_for_all_layer_types(
make_napari_viewer, layer_class, data, ndim
):
"""Test that attributes are in the viewer_state for all napari layer types"""
viewer = make_napari_viewer()
add_layer_by_type(viewer, layer_class, data, visible=True)
layer_animation = Animation(viewer)
# get the state of the layer
layer_state = viewer.layers[0]._get_state()
# remove attributes that arn't captured
psobolewskiPhD marked this conversation as resolved.
Show resolved Hide resolved
layer_state.pop("metadata")
layer_state.pop("data", None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


layer_animation.capture_keyframe()
# get the layer attributes captured to viewer_state
animation_state = layer_animation.key_frames[0].viewer_state.layers[
viewer.layers[0].name
]

assert_layer_state_equal(animation_state, layer_state)


@pytest.mark.parametrize("layer_class, data, ndim", layer_test_data)
def test_animating_all_layer_types(
make_napari_viewer, layer_class, data, ndim
):
"""Test that all napari layer types can be animated"""
viewer = make_napari_viewer()
add_layer_by_type(viewer, layer_class, data, visible=True)
layer_animation = Animation(viewer)
layer_animation.capture_keyframe()
layer_animation.viewer.camera.zoom *= 2
layer_animation.capture_keyframe()
# advance the movie frame, simulating slider movement
layer_animation.set_movie_frame_index(1)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

15 changes: 15 additions & 0 deletions napari_animation/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,18 @@ def pairwise(iterable):
a, b = itertools.tee(iterable)
next(b, None)
return zip(a, b)


def layer_attribute_changed(value, original_value):
"""Recursively check if a layer attribute has changed."""
if isinstance(value, dict):
if (
not isinstance(original_value, dict)
or value.keys() != original_value.keys()
):
return True
for key in value.keys():
if layer_attribute_changed(value[key], original_value[key]):
return True
else:
return not np.array_equal(value, original_value)
psobolewskiPhD marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 10 additions & 4 deletions napari_animation/viewer_state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import napari
import numpy as np

from napari_animation.utils import layer_attribute_changed


@dataclass(frozen=True)
class ViewerState:
Expand Down Expand Up @@ -32,7 +34,7 @@ def from_viewer(cls, viewer: napari.viewer.Viewer):
}
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
)
Expand All @@ -54,9 +56,13 @@ def apply(self, viewer: napari.viewer.Viewer):
layer_attributes = layer.as_layer_data_tuple()[1]
for attribute_name, value in layer_state.items():
original_value = layer_attributes[attribute_name]
# Only set if value differs to avoid expensive redraws
if not np.array_equal(original_value, value):
setattr(layer, attribute_name, value)
# Only setattr if value has changed to avoid expensive redraws
# dicts can hold arrays, e.g. `color`, requiring comparisons of key/value pairs
if layer_attribute_changed(value, original_value):
try:
setattr(layer, attribute_name, value)
except AttributeError:
pass

def render(
self, viewer: napari.viewer.Viewer, canvas_only: bool = True
Expand Down