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

Fix issues with overflows #73

Merged
merged 6 commits into from
Jan 13, 2019
Merged

Fix issues with overflows #73

merged 6 commits into from
Jan 13, 2019

Conversation

nbgl
Copy link
Contributor

@nbgl nbgl commented Dec 19, 2018

Fixes an issue in EncodedNumber.encode and another in EncodedNumber.decode where OverflowErrors occur for valid inputs. This happens when a very big int is coerced to a float but can’t fit. (This may occur even when the intended result is a perfectly valid, finite float.)

We work around this by performing exact arithmetic for as long as possible.

May be related to #62.

@nbgl nbgl added the bug label Dec 19, 2018
@nbgl nbgl self-assigned this Dec 19, 2018
@nbgl nbgl requested a review from hardbyte December 19, 2018 09:58
Copy link
Collaborator

@hardbyte hardbyte left a comment

Choose a reason for hiding this comment

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

Changes all look good to me, minor comments and we need tests for the new utility functions.

phe/util.py Outdated


def round_m_mul_2_pow_e_mul_b_pow_c(m, e, b, c):
"""Compute round(m * 2 ** e * b ** e) for m, e, b, c integers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo in the equation (e instead of c).

Would be good to add a few regression tests checking that this function equals round(m * 2 ** e * b ** e) where it should...

phe/encoding.py Outdated
# a perfectly valid float. We represent scalar as integers
# and avoid coercing to float.
m, e = frexp_int(scalar)
int_rep = round_m_mul_2_pow_e_mul_b_pow_c(m, e,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Variable names should be more explicit - use mantissa and exponent instead of m and e.

Also the util function is a bit of a mouthful, I'd probably go for something more along the lines of compute_float_encoding.

phe/util.py Outdated
@@ -427,3 +429,68 @@ def is_prime(n, mr_rounds=25):
return False
# the actual generic test; give a false prime with probability 2⁻⁵⁰
return miller_rabin(n, mr_rounds)


def int_div_round(a, b):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Function needs tests.

phe/encoding.py Outdated
int_rep = scalar * cls.BASE ** -exponent
else:
# Integer division with correct rounding.
# Can't just to `scalar * cls.BASE ** -exponent` as it
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo to -> do

phe/encoding.py Outdated
# It is possible for scalar and BASE ** -exponent to not be
# representable as float, even when the encoded value is
# a perfectly valid float. We represent scalar as integers
# and avoid coercing to float.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possible to misunderstand your comment here as the scalar you mention is different from the current scoped variable scalar which at this point is a float.

@hardbyte
Copy link
Collaborator

Oh one final comment, there needs to be tests that take each possible encoding path.

@nbgl
Copy link
Contributor Author

nbgl commented Jan 2, 2019

I’ve got rid of a bunch of helper functions by using the fractions package, so these no longer need testing. It also allowed me to get rid of a bunch of branches, so the current tests should suffice.

@nbgl nbgl requested a review from hardbyte January 2, 2019 23:43
Copy link
Collaborator

@hardbyte hardbyte left a comment

Choose a reason for hiding this comment

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

Wow that got a lot simpler! Good stuff.

@nbgl nbgl merged commit 955f8c0 into master Jan 13, 2019
@nbgl nbgl deleted the fix-float-overflows branch January 13, 2019 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants