fix floating point errors in ImageSequenceClip#2518
Open
trygvrad wants to merge 1 commit intoZulko:masterfrom
Open
fix floating point errors in ImageSequenceClip#2518trygvrad wants to merge 1 commit intoZulko:masterfrom
trygvrad wants to merge 1 commit intoZulko:masterfrom
Conversation
aa37964 to
b2b3041
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both #2412 and #2507 relate to comparisons that fail due to floating point precision loss.
In #2412 this leads to the frames being included at the wrong timestep.
In #2507 this leads to the last frame not being included.
For #2412,
self.images_startsis calculated in the following way:The inclusion of
np.finfo(np.float32).epsmakes the dtype of each element 'float32', and this has lower precision than the number it is compared to. In the cases where there is sufficient precision in a float32 fornp.finfo(np.float32).epsto impact the numerical value,self.images_starts[i] <= i/fpsstill holds true, but this is only true for numbers close to one.Removing
-np.finfo(np.float32).epsfixes this error.Note: the
-np.finfo(np.float32).epswas introduced to fix the same error (#464, 167db9f), but the fix was not sufficiently testedThe new test added to test #2412 is inspired by the code in #464
#2507 is related to the duration, calculated as:
which is then compared to
len(self.sequences)/self.fps, which results in a classical floating point error, similar to hownp.sum([1.0/71 for i in range(71)]) == 1is False.Changing the calculation of duration to:
fixes the error, because then the calculation of the duration, and the check of the duration, is calculated in the same way.
There are two new tests:
Which fail on main but pass with this PR.
Note that the details of the failure is system-specific, and different systems will fail at different points (fps), which is why fps values from 1 to 30 are tested.
tests/