search

napari / napari-animation

A napari plugin for making animations
https://napari.github.io/napari-animation/
Other
76 stars 27 forks source link

Make state interpolation work on ViewerState objects #97

Closed alisterburt closed 3 years ago

alisterburt commented 3 years ago

Interpolation of viewer states is currently based on the nested getters and setters which work with dictionaries, not ViewerState objects, meaning we have to first turn our viewerstate into a dictionary with dataclasses.asdict()

https://github.com/napari/napari-animation/blob/f38467593df65e224a0573b2e12f2b7cbcb8348c/napari_animation/frame_sequence.py#L134-L145

This PR (which includes #96) solves #89 by adding dict-like methods __getitem__, keys() and get()) on the ViewerState which allows us to treat it as a nested dictionary and interpolate as usual.

note: I tried to do this first by modifying the interpolation machinery, my attempts were less clean than this. I fully accept that this may be more complicated than necessary but would be really interested to hear what you guys think? @tlambert03 @Fifourche

codecov[bot] commented 3 years ago

Codecov Report

Merging #97 (1227107) into main (f384675) will decrease coverage by 1.43%. The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #97      +/-   ##
==========================================
- Coverage   90.11%   88.68%   -1.44%     
==========================================
  Files          20       20              
  Lines         840      831       -9     
==========================================
- Hits          757      737      -20     
- Misses         83       94      +11
Impacted Files Coverage Δ
napari_animation/_tests/test_interpolation.py 100.00% <ø> (ø)
napari_animation/frame_sequence.py 100.00% <ø> (ø)
napari_animation/interpolation.py 83.33% <ø> (-14.63%) :arrow_down:
napari_animation/utils.py 90.90% <81.25%> (-6.53%) :arrow_down:
napari_animation/key_frame.py 92.53% <90.00%> (-0.45%) :arrow_down:
napari_animation/_tests/test_frame_sequence.py 100.00% <100.00%> (ø)
napari_animation/_tests/conftest.py 96.96% <0.00%> (-3.04%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f384675...1227107. Read the comment docs.

tlambert03 commented 3 years ago

adding dict-like methods getitem, keys() and get()) on the ViewerState which allows us to treat it as a nested dictionary and interpolate as usual.

hmmm... this actually feels a bit stranger to me than just using asdict. I really haven't looked that deeply into the nested_set/nested_get thing, but if that part is pretty set in stone (and it requires a dict), then I'd say just stick with asdict, (which is the standard way to convert a dataclass into a dict), rather than making the dataclass itself have dictlike-characteristics

alisterburt commented 3 years ago

totally fair, I had a feeling that might be the case 🙂 thanks for taking a look!

the nested get/set stuff comes from wanting an easy way to specify interpolation functions for specific attributes which may be arbitrarily deeply nested in the ViewerState - there's some discussion in an 'old' PR if you're interested

Fifourche commented 3 years ago

I agree that this is related to how _interpolate_state is built (nested set/get, and the InterpolationMap format). I guess there would be a good re-thinking to be done then !