napari / napari-animation

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

change fps to duration to account for changes to imageio #182

Closed aelefebv closed 7 months ago

aelefebv commented 11 months ago

Closes: https://github.com/napari/napari-animation/issues/174

aelefebv commented 11 months ago

Alternatively, we could keep fps as a parameter and translate it to duration before passing it to imageio via duration=1000/fps

jni commented 11 months ago

Alternatively, we could keep fps as a parameter and translate it to duration before passing it to imageio via duration=1000/fps

Personally this would be my preference, especially because you could gate that on the imageio version, so there would be no disruption at all to any users (even those with older imageio).

jni commented 11 months ago

I also think it's easier to think about fps

alisterburt commented 11 months ago

Thanks for the PR @aelefebv ! Much appreciated :-)

I agree with Juan here, we should not mirror the imageio API and should instead keep ours consistent with FPS

aelefebv commented 11 months ago

Makes sense to me! Just changed it back to fps and now passing in duration to get_writer.

And thanks again for the great plugin :)

jni commented 10 months ago

Hmmm, perhaps we need a try/except or if/else somewhere to account for multiple imageio versions? The tests are failing with the current changes:

   E       TypeError: FfmpegFormat.Writer._open() got an unexpected keyword argument 'duration'
codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (a9ca7a7) 86.25% compared to head (922e8f2) 86.26%. Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #182 +/- ## ========================================== + Coverage 86.25% 86.26% +0.01% ========================================== Files 26 26 Lines 1011 1012 +1 ========================================== + Hits 872 873 +1 Misses 139 139 ```

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

psobolewskiPhD commented 7 months ago

@jni The duration kwarg is for pillow plugin only (see https://github.com/imageio/imageio/issues/992#issuecomment-1567479878 ) so I just reverted the change for ffmpeg. I think this is good to go, unless you prefer to not have the duration setting in the try: -- could just do it in the imageio call.