-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix out of bounds error #570
Conversation
`iter_chunks` now uses `np.linspace` to generate indices. In addition to fixing the out of bounds error, this also ensures each chunk generates an approximately equal number of indices.
Hmm, it looks like this PR doesn't solve all the cases. I'll keep investigating. |
|
||
pospos = list(range(0, totalsize, chunksize))+[totalsize] | ||
pospos = np.linspace(0, totalsize, nchunks + 1, endpoint=True, dtype=int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intended to add + 1
to nchunks on L70 and add + 1
again in pospos
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Since we're generating endpoint=True
, we need to pass in one more than the number of chunks. I.e. if nchunks
is 1, we tell linspace
to return two points (a start and an end).
As far as I can tell, that last commit fixes all remaining errors on my machine. I've been making heavy use of |
Ok, I've successfully stress-tested this PR by processing several hour-long videos without encountering any errors at all. I believe this PR is ready to merge. The test suite seems to pass, but on my machine
I'm not quite sure why that's happening, but I don't think it's related to this PR. If you have any questions or concerns, let me know. |
@shawwn I don't think those errors are relevant. As long as it passes on Travis, I think it should be ok. |
Ok, cool. In that case, this PR is ready to merge. |
After heavy usage, I still haven't run into any out of bounds errors (or any other problems). |
merging soon ? |
Any idea when this can be merged? This would fix an issue that I'm currently dealing with. |
@mbeacom I still haven't run into any out of bounds errors, and @stonebig @kevinmcalear have requested this be merged. Since this PR is now four months old, can it be accepted? |
(Or @Zulko.) |
@Zulko @mbeacom, could this PR be merged? Several users have asked for this, and after heavy usage I still haven't run into any out of bounds errors or any other problems. I suspect this PR has languished because it changes the bounds checking. In general, it's good to be suspicious of doing this carelessly. But the math is correct now; the reason users have been running into errors is that the math was off in certain corner cases. Another reason is probably that this seems like a magical, unexplained change in try:
result = np.zeros((len(tt),self.nchannels))
indices = frames - self.buffer_startframe
+ if len(self.buffer) < self.buffersize // 2:
+ indices = indices - (self.buffersize // 2 - len(self.buffer) + 1)
result[in_time] = self.buffer[indices]
return result Again, it's good to be hesitant of changes like that, but FWIW I arrived at this solution by running moviepy on a video that breaks, then sat in If other users are running into out of bounds errors with moviepy and just need a fix, I suppose you can apply the above changes manually. But it'd be nice to get this merged. |
Sorry for the delay. I believe most maintainers are too busy right now to give this much thought so we'll have to trust you on this (I hope that was the right decision :) ). |
Closes #19, #246, #269, #281.
iter_chunks
now usesnp.linspace
to generate indices. In additionto fixing the out of bounds error, this also ensures each chunk
generates a uniform number of indices.