-
Notifications
You must be signed in to change notification settings - Fork 26
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.
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
change fps to duration to account for changes to imageio #182
Conversation
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). |
I also think it's easier to think about fps |
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 |
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 :) |
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:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
@jni The |
Closes: #174