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

Add round method to checks base #2931

Merged
merged 4 commits into from
Jan 14, 2019
Merged

Add round method to checks base #2931

merged 4 commits into from
Jan 14, 2019

Conversation

nmuesch
Copy link
Collaborator

@nmuesch nmuesch commented Jan 11, 2019

What does this PR do?

Creates a rounding util method in checks_base that should mostly adhere to how round worked in Py2

Motivation

Python2 and 3 use different rounding strategies and could offer different results. Python 2, in the event of a tie, rounds away from zero.

Python 3 on the other hand, breaks the tie by rounding to the closer even number

Ex:

Py2: round(2.5) -> 3
Py3: round(2.5) -> 2

Using the Decimal library allows us to control the rounding strategy and keeps all checks using the round function able to return the same value as before.

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Feature or bugfix has tests
  • Git history is clean
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo
  • If PR adds a configuration option, it has been added to the configuration file.

Additional Notes

Anything else we should know when reviewing?

@codecov-io
Copy link

codecov-io commented Jan 11, 2019

Codecov Report

Merging #2931 into master will decrease coverage by 9.28%.
The diff coverage is 86.66%.

@@            Coverage Diff            @@
##           master   #2931      +/-   ##
=========================================
- Coverage   84.79%   75.5%   -9.29%     
=========================================
  Files         649      45     -604     
  Lines       36097    3572   -32525     
  Branches     4301     429    -3872     
=========================================
- Hits        30608    2697   -27911     
+ Misses       4215     766    -3449     
+ Partials     1274     109    -1165

@@ -23,6 +24,11 @@ def ensure_unicode(s):
to_string = ensure_unicode if PY3 else ensure_bytes


def round_value(value, sig_digits="0.01", rounding_method=ROUND_HALF_UP):
getcontext().rounding = rounding_method
return float(Decimal(str(value)).quantize(Decimal(sig_digits)))
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to convert to string before passing to decimal

Copy link
Contributor

Choose a reason for hiding this comment

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

You do actually to get proper arithmetic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, there are interesting behaviors when you don't appropriately cast to string. The documentation for Decimal also has all floating point examples in string format - https://docs.python.org/2/library/decimal.html

@@ -23,6 +24,11 @@ def ensure_unicode(s):
to_string = ensure_unicode if PY3 else ensure_bytes


def round_value(value, sig_digits="0.01", rounding_method=ROUND_HALF_UP):
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, sig_digits can be a float.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same consideration as above 🙂

Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -1,6 +1,7 @@
# (C) Datadog, Inc. 2018
# All rights reserved
# Licensed under a 3-clause BSD style license (see LICENSE)
from decimal import ROUND_HALF_UP, Decimal, getcontext
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from decimal import ROUND_HALF_UP, Decimal, getcontext
from decimal import ROUND_HALF_UP, Decimal

@@ -23,6 +24,11 @@ def ensure_unicode(s):
to_string = ensure_unicode if PY3 else ensure_bytes


def round_value(value, sig_digits="0.01", rounding_method=ROUND_HALF_UP):
getcontext().rounding = rounding_method
return float(Decimal(str(value)).quantize(Decimal(sig_digits)))
Copy link
Contributor

Choose a reason for hiding this comment

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

def round_num(value, precision=0):
    return float(Decimal(str(value)).quantize(Decimal('0.{}'.format('0' * precision)), rounding=ROUND_HALF_UP))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like leaving it open to modify the rounding strategy if anyone requires something like Python 3s ROUND_HALF_EVEN function later. I don't think it hurts to leave it in the signature as an optional parameter

I do like the precision component!


class TestRounding():
def test_round_half_up(self):
assert round_value(2.555) == 2.560
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert round_value(2.555) == 2.560
assert round_num(2.5) == 3

zippolyte
zippolyte previously approved these changes Jan 11, 2019
@nmuesch nmuesch merged commit b166efd into master Jan 14, 2019
@nmuesch nmuesch deleted the nick/base_round_util branch January 14, 2019 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants