Skip to content
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

Convert duration with full precision #476

Merged
merged 13 commits into from
Oct 22, 2019

Conversation

terrorfisch
Copy link
Member

@terrorfisch terrorfisch commented Aug 14, 2019

This pull request fixes a duration value change (#475 ) when converting it from float to TimeType(arbitrary precision via fractions) by abusing float to str to get a consistent rounding.

This might lead to unexpected consequences due to float rounding errors with fractional times.

TODO:

  • Add more test cases
  • Quantify performance impact

I think the factor 2 worse performance (with gmpy) is worth the improved ergonomics.

@eendebakpt

@terrorfisch
Copy link
Member Author

Best solution I came up with is to abuse str to get a consistent rounding that doesn't throw of the user.

absolute_error == 0: Return the exact value of the float. 0.8 == 3602879701896397 / 4503599627370496
else: Use absolute error to limit the denominator

str(value) guarantees that all floats have a different result with sensible rounding.this was chosen as a
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@terrorfisch
Copy link
Member Author

terrorfisch commented Oct 21, 2019

Benchmarks indicate a 10% performance hit in "combined" benchmark. For raw computations there is a factor of 2 overhead over gmpy2.

The new code needs ~10µs per numeric operation and ~20µs to convert a float (on my machine).

@terrorfisch
Copy link
Member Author

terrorfisch commented Oct 21, 2019

Before:

Name (time in ms)                                     Min                Max              Mean            StdDev            Median               IQR            Outliers       OPS            Rounds  Iterations
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_time_type_addition_with_float_performance     2.1304 (1.0)       2.4029 (1.0)      2.1588 (1.0)      0.0390 (1.0)      2.1450 (1.0)      0.0113 (1.0)         47;67  463.2255 (1.0)         468           1
test_time_type_from_float_performance              5.4950 (2.58)     32.0087 (13.32)    5.7039 (2.64)     2.0004 (51.34)    5.5399 (2.58)     0.0336 (2.97)         1;19  175.3199 (0.38)        175           1
test_time_type_mul_performance                     5.8042 (2.72)      6.1170 (2.55)     5.8695 (2.72)     0.0553 (1.42)     5.8441 (2.72)     0.0899 (7.96)         45;2  170.3719 (0.37)        171           1
test_time_type_addition_performance                5.9069 (2.77)      6.2421 (2.60)     5.9923 (2.78)     0.0579 (1.49)     5.9740 (2.79)     0.0773 (6.84)         51;3  166.8818 (0.36)        167           1
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

After:

Name (time in ms)                                      Min                Max               Mean            StdDev             Median               IQR            Outliers      OPS            Rounds  Iterations
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_time_type_mul_performance                     11.2950 (1.0)      11.6003 (1.0)      11.3918 (1.0)      0.0682 (1.0)      11.3823 (1.0)      0.0997 (1.0)          22;1  87.7828 (1.0)          88           1
test_time_type_addition_performance                11.5525 (1.02)     12.6481 (1.09)     12.0731 (1.06)     0.2080 (3.05)     12.0662 (1.06)     0.2563 (2.57)         20;2  82.8286 (0.94)         82           1
test_time_type_from_float_performance              20.3693 (1.80)     47.5711 (4.10)     21.3644 (1.88)     3.8722 (56.78)    20.7756 (1.83)     0.3648 (3.66)          1;2  46.8069 (0.53)         48           1
test_time_type_addition_with_float_performance     32.0184 (2.83)     40.9395 (3.53)     32.9890 (2.90)     1.7793 (26.09)    32.4464 (2.85)     0.4329 (4.34)          3;5  30.3131 (0.35)         27           1
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

# Conflicts:
#	tests/utils/time_type_tests.py
+ Make random usage deterministic
@coveralls
Copy link

coveralls commented Oct 21, 2019

Pull Request Test Coverage Report for Build 1866

  • 124 of 140 (88.57%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 89.875%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qupulse/expressions.py 2 3 66.67%
qupulse/utils/types.py 118 133 88.72%
Totals Coverage Status
Change from base Build 1856: -0.03%
Covered Lines: 26055
Relevant Lines: 28610

💛 - Coveralls

@terrorfisch terrorfisch merged commit 4dc1092 into master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants