Skip to content

Commit b680c28

Browse files
jamesbajamesba
authored
Merge pull request #27 from bbc/jamesba-normaliserounddown
Fixed bug that caused incorrect normalisation on some timestamps
2 parents aa7e462 + a42239f commit b680c28

7 files changed

Lines changed: 140 additions & 31 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,17 @@
11
# mediatimestamp Changelog
22

3+
## 1.5.1
4+
- Fixed bug in `TimeOffset.to_count` (both versions) that caused
5+
incorrect results when rounding down when the denominator of the
6+
rate properly divided neither the seconds part nor the nanoseconds
7+
part of the timestamp, nor the number of nanoseconds in a second,
8+
but did properly divide the whole timestamp expressed in
9+
nanoseconds. This bug has been present for some time but had not
10+
previously been seen because there was no test coverage for
11+
non-nearest rounding and none of our tests used rates with prime
12+
denominators other than 2.
13+
- Switched test runner in tox from nose2 to unittest
14+
315
## 1.5.0
416
- Added normalisation function for immutable.TimeRange
517

‎mediatimestamp/immutable.py‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -285,10 +285,12 @@ def to_count(self, rate_num, rate_den=1, rounding=ROUND_NEAREST):
285285
# = (off_sec x rate_num / rate_den) + (off_nsec x rate_num) / rate_den)
286286
# = {f1} + {f2}
287287
# reduce {f1} as follows: a*b/c = ((a/c)*c + a%c) * b) / c = (a/c)*b + (a%c)*b/c
288+
# then combine the f1 and f2 nanosecond values before division by rate_den to avoid loss of precision when
289+
# off_sec and off_nsec are not multiples of rate_den, but (off_sec + off_nsec) is
288290
f1_whole = (abs_off.sec // rate_den) * rate_num
289-
f1_nsec = (abs_off.sec % rate_den) * rate_num * self.MAX_NANOSEC // rate_den
290-
f2_nsec = abs_off.ns * rate_num // rate_den
291-
return self.sign * (f1_whole + (f1_nsec + f2_nsec) // self.MAX_NANOSEC)
291+
f1_dennsec = (abs_off.sec % rate_den) * rate_num * self.MAX_NANOSEC
292+
f2_dennsec = abs_off.ns * rate_num
293+
return self.sign * (f1_whole + (f1_dennsec + f2_dennsec) // (rate_den * self.MAX_NANOSEC))
292294

293295
def to_phase_offset(self, rate_num, rate_den=1):
294296
"""Return the smallest positive TimeOffset such that abs(self - returnval) represents an integer number of

‎mediatimestamp/mutable.py‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -251,10 +251,12 @@ def to_count(self, rate_num, rate_den=1, rounding=ROUND_NEAREST):
251251
# = (off_sec x rate_num / rate_den) + (off_nsec x rate_num) / rate_den)
252252
# = {f1} + {f2}
253253
# reduce {f1} as follows: a*b/c = ((a/c)*c + a%c) * b) / c = (a/c)*b + (a%c)*b/c
254+
# then combine the f1 and f2 nanosecond values before division by rate_den to avoid loss of precision when
255+
# off_sec and off_nsec are not multiples of rate_den, but (off_sec + off_nsec) is
254256
f1_whole = (abs_off.sec // rate_den) * rate_num
255-
f1_nsec = (abs_off.sec % rate_den) * rate_num * self.MAX_NANOSEC // rate_den
256-
f2_nsec = abs_off.ns * rate_num // rate_den
257-
return self.sign * (f1_whole + (f1_nsec + f2_nsec) // self.MAX_NANOSEC)
257+
f1_dennsec = (abs_off.sec % rate_den) * rate_num * self.MAX_NANOSEC
258+
f2_dennsec = abs_off.ns * rate_num
259+
return self.sign * (f1_whole + (f1_dennsec + f2_dennsec) // (rate_den * self.MAX_NANOSEC))
258260

259261
def to_millisec(self, rounding=ROUND_NEAREST):
260262
use_rounding = rounding

‎setup.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818

1919
# Basic metadata
2020
name = 'mediatimestamp'
21-
version = '1.5.0'
21+
version = '1.5.1'
2222
description = 'A timestamp library for high precision nanosecond timestamps'
2323
url = 'https://github.com/bbc/rd-apmm-python-lib-mediatimestamp'
2424
author = 'James P. Weaver'

‎tests/test_immutable.py‎

Lines changed: 58 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -60,21 +60,66 @@ def test_from_timeoffset(self):
6060

6161
def test_normalise(self):
6262
tests_ts = [
63-
(TimeOffset(0, 0), (30000, 1001), TimeOffset(0, 0)),
64-
(TimeOffset(1001, 0), (30000, 1001), TimeOffset(1001, 0)),
65-
(TimeOffset(1001, 1001.0/30000/2*1000000000), (30000, 1001), TimeOffset(1001, 0)),
66-
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1), (30000, 1001), TimeOffset(1001, 1001.0/30000*1000000000)),
67-
(TimeOffset(1001, 1001.0/30000/2*1000000000, -1), (30000, 1001), TimeOffset(1001, 0, -1)),
68-
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1, -1), (30000, 1001),
69-
TimeOffset(1001, 1001.0/30000*1000000000, -1))
63+
(TimeOffset(0, 0), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
64+
TimeOffset(0, 0)),
65+
(TimeOffset(1001, 0), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
66+
TimeOffset(1001, 0)),
67+
(TimeOffset(1001, 1001.0/30000/2*1000000000), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
68+
TimeOffset(1001, 0)),
69+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
70+
TimeOffset(1001, 1001.0/30000*1000000000)),
71+
(TimeOffset(1001, 1001.0/30000/2*1000000000, -1), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
72+
TimeOffset(1001, 0, -1)),
73+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1, -1), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
74+
TimeOffset(1001, 1001.0/30000*1000000000, -1)),
75+
(TimeOffset(1521731233, 320000000), Fraction(25, 3), TimeOffset.ROUND_NEAREST,
76+
TimeOffset(1521731233, 320000000)),
77+
(TimeOffset(0, 0), Fraction(30000, 1001), TimeOffset.ROUND_UP,
78+
TimeOffset(0, 0)),
79+
(TimeOffset(1001, 0), Fraction(30000, 1001), TimeOffset.ROUND_UP,
80+
TimeOffset(1001, 0)),
81+
(TimeOffset(1001, 1001.0/30000/2*1000000000), Fraction(30000, 1001), TimeOffset.ROUND_UP,
82+
TimeOffset(1001, 1001.0/30000*1000000000)),
83+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1), Fraction(30000, 1001), TimeOffset.ROUND_UP,
84+
TimeOffset(1001, 1001.0/30000*1000000000)),
85+
(TimeOffset(1001, 1001.0/30000/2*1000000000, -1), Fraction(30000, 1001), TimeOffset.ROUND_UP,
86+
TimeOffset(1001, 0, -1)),
87+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1, -1), Fraction(30000, 1001), TimeOffset.ROUND_UP,
88+
TimeOffset(1001, 0, -1)),
89+
(TimeOffset(1521731233, 320000000), Fraction(25, 3), TimeOffset.ROUND_UP,
90+
TimeOffset(1521731233, 320000000)),
91+
(TimeOffset(0, 0), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
92+
TimeOffset(0, 0)),
93+
(TimeOffset(1001, 0), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
94+
TimeOffset(1001, 0)),
95+
(TimeOffset(1001, 1001.0/30000/2*1000000000), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
96+
TimeOffset(1001, 0)),
97+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
98+
TimeOffset(1001, 0)),
99+
(TimeOffset(1001, 1001.0/30000/2*1000000000, -1), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
100+
TimeOffset(1001, 1001.0/30000*1000000000, -1)),
101+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1, -1), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
102+
TimeOffset(1001, 1001.0/30000*1000000000, -1)),
103+
(TimeOffset(1521731233, 320000000), Fraction(25, 3), TimeOffset.ROUND_DOWN,
104+
TimeOffset(1521731233, 320000000)),
70105
]
71106

72-
for t in tests_ts:
73-
with self.subTest(t=t):
74-
r = t[0].normalise(t[1][0], t[1][1])
75-
self.assertEqual(r, t[2],
76-
msg=("{!r}.normalise({}, {}) == {!r}, expected {!r}"
77-
.format(t[0], t[1][0], t[1][1], r, t[2])))
107+
n = 0
108+
for (input, rate, rounding, expected) in tests_ts:
109+
# Nb. subTest will add a printout of all its kwargs to any error message generated
110+
# by a failure within it. The variable n is being used here to ensure that the index
111+
# of the current test within tests_ts is printed on any failure. (Nb. only works with
112+
# python3 unittest test runner)
113+
with self.subTest(test_data_index=n,
114+
input=input,
115+
rate=rate,
116+
rounding=rounding,
117+
expected=expected):
118+
n += 1
119+
r = input.normalise(rate.numerator, rate.denominator, rounding=rounding)
120+
self.assertEqual(r, expected,
121+
msg=("{!r}.normalise({}, {}, rounding={}) == {!r}, expected {!r}"
122+
.format(input, rate.numerator, rate.denominator, rounding, r, expected)))
78123

79124
def test_hash(self):
80125
self.assertEqual(hash(TimeOffset(0, 0)), hash(TimeOffset(0, 0)))

‎tests/test_mutable.py‎

Lines changed: 58 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,19 +16,31 @@
1616

1717
import unittest
1818
import mock
19+
import contextlib
1920

2021
from datetime import datetime
2122
from dateutil import tz
23+
from fractions import Fraction
2224

2325
from mediatimestamp.mutable import Timestamp, TimeOffset, TsValueError, TimeRange
2426

27+
28+
@contextlib.contextmanager
29+
def dummysubtest(*args, **kwargs):
30+
yield None
31+
32+
2533
if PY2:
2634
BUILTINS = "__builtin__"
2735
else:
2836
BUILTINS = "builtins"
2937

3038

3139
class TestTimeOffset(unittest.TestCase):
40+
def setUp(self):
41+
if PY2:
42+
self.subTest = dummysubtest
43+
3244
def test_MAX_NANOSEC(self):
3345
self.assertEqual(TimeOffset.MAX_NANOSEC, 1000000000)
3446

@@ -45,22 +57,59 @@ def test_from_timeoffset(self):
4557

4658
def test_normalise(self):
4759
tests_ts = [
48-
(TimeOffset(0, 0).normalise(30000, 1001),
60+
(TimeOffset(0, 0), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
4961
TimeOffset(0, 0)),
50-
(TimeOffset(1001, 0).normalise(30000, 1001),
62+
(TimeOffset(1001, 0), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
5163
TimeOffset(1001, 0)),
52-
(TimeOffset(1001, 1001.0/30000/2*1000000000).normalise(30000, 1001),
64+
(TimeOffset(1001, 1001.0/30000/2*1000000000), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
65+
TimeOffset(1001, 0)),
66+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
67+
TimeOffset(1001, 1001.0/30000*1000000000)),
68+
(TimeOffset(1001, 1001.0/30000/2*1000000000, -1), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
69+
TimeOffset(1001, 0, -1)),
70+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1, -1), Fraction(30000, 1001), TimeOffset.ROUND_NEAREST,
71+
TimeOffset(1001, 1001.0/30000*1000000000, -1)),
72+
(TimeOffset(1521731233, 320000000), Fraction(25, 3), TimeOffset.ROUND_NEAREST,
73+
TimeOffset(1521731233, 320000000)),
74+
(TimeOffset(0, 0), Fraction(30000, 1001), TimeOffset.ROUND_UP,
75+
TimeOffset(0, 0)),
76+
(TimeOffset(1001, 0), Fraction(30000, 1001), TimeOffset.ROUND_UP,
5377
TimeOffset(1001, 0)),
54-
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1).normalise(30000, 1001),
78+
(TimeOffset(1001, 1001.0/30000/2*1000000000), Fraction(30000, 1001), TimeOffset.ROUND_UP,
5579
TimeOffset(1001, 1001.0/30000*1000000000)),
56-
(TimeOffset(1001, 1001.0/30000/2*1000000000, -1).normalise(30000, 1001),
80+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1), Fraction(30000, 1001), TimeOffset.ROUND_UP,
81+
TimeOffset(1001, 1001.0/30000*1000000000)),
82+
(TimeOffset(1001, 1001.0/30000/2*1000000000, -1), Fraction(30000, 1001), TimeOffset.ROUND_UP,
83+
TimeOffset(1001, 0, -1)),
84+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1, -1), Fraction(30000, 1001), TimeOffset.ROUND_UP,
5785
TimeOffset(1001, 0, -1)),
58-
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1, -1).normalise(30000, 1001),
59-
TimeOffset(1001, 1001.0/30000*1000000000, -1))
86+
(TimeOffset(1521731233, 320000000), Fraction(25, 3), TimeOffset.ROUND_UP,
87+
TimeOffset(1521731233, 320000000)),
88+
(TimeOffset(0, 0), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
89+
TimeOffset(0, 0)),
90+
(TimeOffset(1001, 0), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
91+
TimeOffset(1001, 0)),
92+
(TimeOffset(1001, 1001.0/30000/2*1000000000), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
93+
TimeOffset(1001, 0)),
94+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
95+
TimeOffset(1001, 0)),
96+
(TimeOffset(1001, 1001.0/30000/2*1000000000, -1), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
97+
TimeOffset(1001, 1001.0/30000*1000000000, -1)),
98+
(TimeOffset(1001, 1001.0/30000/2*1000000000 + 1, -1), Fraction(30000, 1001), TimeOffset.ROUND_DOWN,
99+
TimeOffset(1001, 1001.0/30000*1000000000, -1)),
100+
(TimeOffset(1521731233, 320000000), Fraction(25, 3), TimeOffset.ROUND_DOWN,
101+
TimeOffset(1521731233, 320000000)),
60102
]
61103

62-
for t in tests_ts:
63-
self.assertEqual(t[0], t[1])
104+
for (input, rate, rounding, expected) in tests_ts:
105+
with self.subTest(input=input,
106+
rate=rate,
107+
rounding=rounding,
108+
expected=expected):
109+
r = input.normalise(rate.numerator, rate.denominator, rounding=rounding)
110+
self.assertEqual(r, expected,
111+
msg=("{!r}.normalise({}, {}, rounding={}) == {!r}, expected {!r}"
112+
.format(input, rate.numerator, rate.denominator, rounding, r, expected)))
64113

65114
def test_hash(self):
66115
self.assertEqual(hash(TimeOffset(0, 0)), hash(TimeOffset(0, 0)))

‎tox.ini‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,8 @@
77
envlist = py27, py3
88

99
[testenv]
10-
commands = nose2 --with-coverage --coverage-report=annotate --coverage-report=term --coverage=mediatimestamp
10+
commands = python -m unittest discover -s tests
1111
deps =
12-
nose2
1312
mock
1413
hypothesis
1514

0 commit comments

Comments
 (0)