Bugfixes for fractions() and decimals() #777

Zac-HD · 2017-08-12T07:36:07Z

Fixes decimals() fails on a range entirely between two integers #739, which is actually caused by a bug in fractions() - casting bounds to fraction and upgrading dm_func resolves the issue.
Consequently, we also have to fix our handling of max_denominator in fractions() which only worked because the other bounds didn't.
Fixes FailedHealthCheck for decimals(places=X, allow_nan=False, allow_infinity=False) #725 by ensuring our internal strategy for fixed-place decimals only draws values that are representable in the current precision context.
Adds regression tests for the above
Adds a simple fuzz-test for fractions() and decimals() to catch similar problems in future.
Instead of giving incorrect output or raising an assertion in integers(), raise InvalidArgument for non-integer intervals that do not contain any integers (as already happens for integer intervals).

DRMacIver

Thanks for looking into this!

I've gone through it with a pedant-toothed comb because numerics and there are a bunch of issues with the implementation, at least one of which I'm surprised the testing didn't caught so there might be some limitation to the testing that I'm not currently spotting that means it's testing less than it looks like.

The underlying diagnosis of the problem looks sound though.

DRMacIver · 2017-08-13T09:46:37Z

src/hypothesis/strategies.py

+        try:
+            if min_value is not None:
+                assert min_value <= min_value.quantize(factor)
+        except (AssertionError, InvalidOperation):


I'm very unkeen on the use of an assertion for control flow. In theory you can turn assertions off (though it would be weird to do so when running Hypothesis I admit), and it's also just kinda ugly and non-intuitive.

DRMacIver · 2017-08-13T09:47:15Z

src/hypothesis/strategies.py

+                assert min_value <= min_value.quantize(factor)
+        except (AssertionError, InvalidOperation):
+            raise InvalidArgument(
+                'min_value=%r is incompatible with places=%r and decimal '


I don't really understand this error message, and I think I'd be even less likely to do so if I received it in the wild. Why is it incompatible?

DRMacIver · 2017-08-13T09:49:53Z

tests/cover/test_direct_strategies.py

+    with pytest.raises(InvalidArgument):
+        ds.decimals(min_value=b, places=10).example()
+    with pytest.raises(InvalidArgument):
+        ds.decimals(max_value=b, places=10).example()


I find it slightly weird that a max_value can be too large (or, conversely, that a min_value can be too small). I feel like this should be equivalent to the relevant bound being None. I'm not wed to this position, just registering a vague dissatisfaction.

DRMacIver · 2017-08-13T09:53:13Z

src/hypothesis/strategies.py

@@ -214,7 +214,7 @@ def integers(min_value=None, max_value=None):
    max_int_value = None
    if max_value is not None:
        max_int_value = int(max_value)
-        if max_int_value != max_value and max_value < 0:
+        if max_int_value != max_value and max_value < 0:  # pragma: no cover


Why is this no cover?

DRMacIver · 2017-08-13T09:53:44Z

src/hypothesis/strategies.py

+        try:
+            min_value = Fraction(min_value)
+        except (TypeError, ValueError):
+            raise InvalidArgument('min_value=%r to Fraction' % (min_value,))


This isn't a very informative error message...

DRMacIver · 2017-08-13T10:28:03Z

src/hypothesis/strategies.py

+        return val
+
+    if min_value is not None and max_value is not None and \
+            not (min_value <= clip_denominator(min_value) <= max_value):


If you take my suggestion above then this check becomes simpler: After round min_value up and max_value down, you simply check again whether min_value <= max_value.

DRMacIver · 2017-08-13T10:42:42Z

docs/changes.rst

+
+* Fixed-point decimals would often fail with healthcheck errors due to changes
+  in the underlying distribution of integers.  Calculating the correct bound
+  when none is given fixes :issue:`725`.


Nit: None, not none

DRMacIver · 2017-08-13T10:43:09Z

docs/changes.rst

+This is a bugfix release for the :func:`~hypothesis.strategies.fractions`
+and :func:`~hypothesis.strategies.decimals` strategies.
+
+* Fixed-point decimals would often fail with healthcheck errors due to changes


When written out like this health check should probably be two words.

DRMacIver · 2017-08-13T10:44:14Z

tests/cover/test_numerics.py

 from hypothesis.internal.compat import float_to_decimal


+@given(data())
+def test_fuzz_fractions_bounds(data):


I like the addition of this (and the corresponding one for decimals)

DRMacIver · 2017-08-13T10:44:37Z

tests/cover/test_numerics.py

+def test_fuzz_fractions_bounds(data):
+    denom = data.draw(none() | integers(1, 20))
+    fracs = none() | fractions(max_denominator=denom) \
+        | fractions('1/99', '1/2', denom)


It makes me sad that there's no equivalent to @example for data. I've still yet to figure out a good API for it.

mulkieran · 2017-08-14T13:53:42Z

src/hypothesis/strategies.py


        return builds(
            Fraction,
            integers(min_value=min_num, max_value=max_num),
            just(denom)
        )

-    return denominator_strategy.flatmap(dm_func)
+    if max_denominator is None:


For clarity, move this block of code above dm_func definition? All the lines down to 1037 could coneivably go above it.

Also, again for clarity, an else might actually help, as:

if max_denominator is None: ... else: ...

Otherwise, the fact that 1022 and 1036 are duplicates seems just a little odder.

We .flatmap(dm_func) in this block, so it can't move down. I can add some comments if that would help - I found dealing with the simple case and then doing the other validation clarified things by removing many conditionals, but maybe I've just spent a few days in the area recently 😄

Line 1043 won't work correctly if max_denominator is None?

frac.limit_denominator(None) is a TypeError, at least under Python 3.

I could juggle it into a different order with much greater use of if ... is not None: conditionals, but this was much easier to follow - the just(min_value) case is repeated, but the rest falls into a clear sequence.

mulkieran · 2017-08-14T14:34:36Z

src/hypothesis/strategies.py

+
+    if min_value is not None and min_value == max_value:
+        return just(min_value)
+    return integers(1, max_denominator).flatmap(dm_func)\


I do not understand how the changes in dm_func and the corresponding use of limit_denominator fix anything that was broken previously. It is clear what they do, but what do they fix?

There were two intertwined problems:

The value bounds might not be integers. If they're close together, integers() might be given an interval with no integers in it and therefore fail. To correct this, we convert our bounds to fractions up-front and do a little more algebra in dm_func.

Now that we're doing the extra algebra, dm_func can actually increase our denominator - potentially past the max_denominator bound. We therefore use the limit_denominator to clip it to the nearest fraction with at most that denominator, which is always within bounds by construction.

Of course the value bounds might not be integers. Indeed, the call to integers() might be given an interval with no integers in it. In that case, dm_func(<some denom>) will return a strategy that doesn't produce any values. How is that a bug?

It's a bug because fractions() was given valid bounds, and the strategy is meant to produce values.

DRMacIver · 2017-08-14T14:33:08Z

src/hypothesis/strategies.py

@@ -205,6 +205,9 @@ def integers(min_value=None, max_value=None):
    from hypothesis.searchstrategy.numbers import IntegersFromStrategy, \
        BoundedIntStrategy, WideRangeIntStrategy

+    # Why not a simpler implmentation?  eg:
+    #   `min_int_value = None if min_value is None else ceil(min_value)`
+    # Large values then misbehave under Python 2, so we're inelegant for now


What does misbehave mean here?

Cause my fuzz-tests to fail, really. I've poked at it a bit and decided that it's working and not inelegant enough to make me want to trace Python2 int bounding issues.

DRMacIver · 2017-08-14T14:41:44Z

src/hypothesis/strategies.py

+                              '=%r' % (min_value, max_denominator))
+    if max_value is not None and max_value.denominator > max_denominator:
+        raise InvalidArgument('max_value=%r incompatible with max_denominator'
+                              '=%r' % (max_value, max_denominator))


I kinda feel like this is a breaking change unfortunately...

It's true that this would not previous raise an exception, but nor has it ever worked correctly - if you tried, the denominator bound would be respected but the value bounds would not!

There are some cases where with a value-bound-denominator greater than the max_denominator, no examples are possible: eg fractions('1/3', '2/3', 1) has no valid output and nor has fractions(.5, .5, 1).

I see three choices:

Allow some examples to be outside the specified bounds, or allow bounds with no possible examples.

Validate the value-bounds with respect to the precision bound. (I've done this because I like it - the way the various intervals all fall into place is lovely). Note that value bounds may be unrepresentable with fractions of max_denominator; it all still works perfectly.

Try to validate whether there are any possible outputs in cases where a bound has too high a denominator. If max_value - min_value >= Fraction(1, max_denominator), this is trivially true. If not, I haven't yet been able to get possible example denominator values in less than linear time of the greatest denominator (by checking each). The trivial case of equal value bounds with an illegal denominator is obvious but not much use.

The whole point of this pull is that I don't like option one, and three is similar but less consistent!

mulkieran · 2017-08-15T13:25:39Z

src/hypothesis/strategies.py

@@ -982,32 +987,61 @@ def fractions(min_value=None, max_value=None, max_denominator=None):

    If max_denominator is not None then the absolute value of the denominator
    of any generated values is no greater than max_denominator. Note that
-    max_denominator must be at least 1.
+    max_denominator must be None or a positive integer, and the denominator


I think you're changing the semantics radically. In the previous version of this method, the form of the two bounds was completely orthogonal to the choice of denominator. They constituted bounds, upper and lower limits, and carried no further information. I think that was a good choice.

See my response to @DRMacIver above - it was broken before and alternatives are worse 😞

I don't believe that you have demonstrated that the current version is broken in any fundamental way. It is probably easy to return an error in the case where the arguments specified can not yield any values, and that would probably be a good idea.

Try fractions(Fraction(1, 4), Fraction(1, 2), 4).example() on master - you'll see some assertion errors from within integers() even though all the bounds are compatible.

Having fixed that, I actually can't determine in the general case whether there are any legal values when the value bounds are at higher precision than allowed for examples. I'd be very happy to be proved wrong here!

What I see is a bug in the integers() strategy that needs to be fixed (that has been latent for a while).

I think we're probably spending a lot of time worrying about a niche case that doesn't matter all that much and it's OK to handle a bit suboptimally.

Here's a proposed simple solution:

We stop worrying about catching conflicts between bounds and max_denominator up front and allow unsatisfiable tests to fail at runtime.

We do this by going back to the proposed clipping behaviour - if when rounding we get something outside the bounds, we snap to the nearest bound.

If the bound in question has too large a denominator, we filter out the example with assume and allow example generation to try again.

I think that handles all the happy paths well and degrades gracefully in the presence of hard or impossible to satisfy constraints.

How does that sound?

I'm still struggling to understand precisely what problem it is the purpose of these changes to fix.

I'm unconvinced that the change to integers() strategy is not a first step in the wrong direction.

I'm going to ask my former professor, retired, about the problem of deciding whether there exists a rational number of a designated denominator between two rational bounds. He's a an algebraist, he might be interested. But that's orthogonal to the main problem, AFAIAC.

deciding whether there exists a rational number of a designated denominator between two rational bounds

Note that the problem isn't a single designated denominator - that's easy. You just multiply both bounds by the denominator, round the bottom up to an integer and the top down to an integer, and if bounds are still in the right order then you've got a rational of that denominator between them (anything in that integer range divided by the denominator) and if they're not then none can exist..

The problem scenario is finding one of bounded denominator without checking every value <= n.

Yes. I gave him a full description of the problem :) But I'm not optimistic that there is any insight that can be offered beyond the obvious that iff

the max denominator is less than the denominator of each endpoint, AND

the range between the endpoints is less than 1/the max denominator

then it may be computationally expensive to decide if there is an example.

How does that [simple solution] sound?

In any case that mine doesn't cover (see below), this will avoid the filter in literally less than one in several million cases. Will push after rebasing.

I'm still struggling to understand precisely what problem it is the purpose of these changes to fix.

If the min_value or max_value were not integers (and we didn't enforce this, so it's reasonable to assume that fractions are supported), the generated values were not properly bounded. In some cases, this would also trigger an assertion in integers() which I have replaced with a validation call. Plus consequential changes to maintain our max_denominator invariant.

The problem scenario is finding one of bounded denominator without checking every value <= n.

I think it's reasonable to bail out with an explicit error if there is no such value with denominator between n and n-1000. Does that fit with our compatibility commitment? I think this check is exhaustive for almost all user inputs, and not too expensive at very high precision.

Hypothesis 3.12 made very large integers more common, which in turn caused errors due to precision limits. Setting a default bounding value from the precison and checking user bounds should fix this for good.

mulkieran · 2017-08-16T14:07:36Z

I think it would be helpful to have a super-comment for this PR, stating what it accomplishes.

Zac-HD · 2017-08-16T14:34:05Z

(moved to top)

mulkieran · 2017-08-16T14:41:23Z

I'm sorry I was unspecific. I think a super-comment should go at the top, where it is easy to find.

DRMacIver · 2017-08-16T15:44:57Z

I think everyone (me especially included) is getting confused by this pull request and getting frustrated as a result. I know that I've lost the plot ab it about what it does and what it fixes.

I think part of the problem here is that it's fixing multiple different issues, some dependent and some orthogonal. In particular we've got:

A precision related fix for decimals.
Assertion errors in integers with fractional bounds when the result is impossible.
An update to fractions to not trigger the case in 2.
A knock-on change to how we handle the interaction of min/max bounds and denominator bounds

Even though it's a relatively small pull request by line count there are enough moving parts that I think it would benefit from those issues being split up into their own separate pull requests - maybe bundling 3 and 4 into one, but certainly splitting out 1 and 2.

On top of that, the issues it's dealing with are super-fiddly. I think it would probably help (both here and in general when the issue is not super obvious) if bug fix pull requests came with a short explanation of what goes wrong, why it's wrong (i.e. underlying cause) and what the new behaviour is.

PS. Yes I'm aware there's a certain amount of hypocrisy in asking for smaller pull requests given my currently open huge one. Sorry.

Zac-HD · 2017-08-16T23:57:10Z

Sounds good - I'll cherry-pick this into separate pulls to deal with integers, decimals, and fractions.

Zac-HD force-pushed the decimals-strategy branch 8 times, most recently from ae1a38c to 72339ed Compare August 13, 2017 10:16

DRMacIver requested changes Aug 13, 2017

View reviewed changes

Zac-HD force-pushed the decimals-strategy branch 3 times, most recently from c6d5b8d to 95a1a35 Compare August 14, 2017 14:02

mulkieran requested changes Aug 14, 2017

View reviewed changes

mulkieran reviewed Aug 14, 2017

View reviewed changes

DRMacIver requested changes Aug 14, 2017

View reviewed changes

mulkieran reviewed Aug 15, 2017

View reviewed changes

Zac-HD added 5 commits August 16, 2017 14:33

Comment, validation on integers() bounding

8f43159

Fix implementation of decimals(places=X)

5fe0242

Hypothesis 3.12 made very large integers more common, which in turn caused errors due to precision limits. Setting a default bounding value from the precison and checking user bounds should fix this for good.

Handle fractions() bounds correctly

ce210ec

Fuzz and regression tests for numeric bounds

56493dd

Add changelog, bump version

78daba2

Zac-HD force-pushed the decimals-strategy branch from 95a1a35 to 78daba2 Compare August 16, 2017 04:42

Zac-HD closed this Aug 16, 2017

This was referenced Aug 17, 2017

Bugfix for fixed-point decimals strategy #789

Merged

Improved validation for integers() bounds #790

Merged

Zac-HD mentioned this pull request Aug 17, 2017

Correct bounds handling for fractions() #791

Merged

Bugfixes for fractions() and decimals() #777

Bugfixes for fractions() and decimals() #777

Conversation

Zac-HD commented Aug 12, 2017 • edited

DRMacIver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mulkieran Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD Aug 15, 2017 • edited

Choose a reason for hiding this comment

mulkieran Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DRMacIver Aug 16, 2017 • edited

Choose a reason for hiding this comment

mulkieran Aug 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mulkieran commented Aug 16, 2017

Zac-HD commented Aug 16, 2017 • edited

mulkieran commented Aug 16, 2017

DRMacIver commented Aug 16, 2017 • edited

Zac-HD commented Aug 16, 2017

Zac-HD commented Aug 12, 2017 •

edited

mulkieran Aug 15, 2017 •

edited

Zac-HD Aug 15, 2017 •

edited

mulkieran Aug 15, 2017 •

edited

DRMacIver Aug 16, 2017 •

edited

mulkieran Aug 16, 2017 •

edited

Zac-HD commented Aug 16, 2017 •

edited

DRMacIver commented Aug 16, 2017 •

edited