Closed alisterburt closed 3 years ago
Merging #35 (0190da1) into main (5504078) will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## main #35 +/- ##
=======================================
Coverage 34.07% 34.07%
=======================================
Files 12 12
Lines 537 537
=======================================
Hits 183 183
Misses 354 354
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 5504078...0190da1. Read the comment docs.
Hmm, it's probably worth it, but maybe @jni can give a second opinion before we decide
could consider an extra? napari-animation[export]
or something like that? but if it's basically the primary task for this package, then maybe should just depend on it
@guiwitz curious if you think this is a good idea or not, based on your comment https://napari.zulipchat.com/#narrow/stream/212875-general/topic/animations/near/229700173
I actually didn't know one could install ffmpeg via pip (must be relatively new?), I always used conda for that. Given that this makes installation much simpler (and not forcing people to use conda) I'm all for including it in the requirements!
I actually didn't know one could install ffmpeg via pip (must be relatively new?), I always used conda for that. Given that this makes installation much simpler (and not forcing people to use conda) I'm all for including it in the requirements!
Ok great, I'm in favor then! Thanks for giving another opinion. Let's go with it. We can also improve some of the gif etc saving that you mentioned in other PRs too @guiwitz
Thanks for adding this @alisterburt!! I will merge now
Adding imageio-ffmpeg as a requirement would reduce the need to install another thing for users who want it to just work. The tests in #34 currently fail beause of this - this would fix that. If we don't want to add this as a requirement, I can make sure separate 'test time' reqs are working as they do in napari